Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Fix elements scrolling with elementScrollTo in integration tests #3138

Merged
merged 2 commits into from
Jan 29, 2025
Merged

chore: Fix elements scrolling with elementScrollTo in integration tests #3138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17f2d91
chore: Fix elements scrolling with elementScrollTo in integration tests
Al-Dani Nov 28, 2024
9b483f1
chore: Remove scrolling in select test
Al-Dani Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/app-layout/__integ__/app-layout-drawers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { viewports } from './constants';
import { testIf } from './utils';

const wrapper = createWrapper().findAppLayout();
import vrDrawerStyles from '../../../lib/components/app-layout/visual-refresh/styles.selectors.js';
import vrToolbarDrawerStyles from '../../../lib/components/app-layout/visual-refresh-toolbar/drawer/styles.selectors.js';

class AppLayoutDrawersPage extends BasePageObject {
async openFirstDrawer() {
Expand Down Expand Up @@ -242,7 +244,14 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme =>
setupTest({ theme }, async page => {
await page.openFirstDrawer();
const resizeHandleBefore = await page.getResizeHandlePosition();
await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 });
const scrollableContainer =
theme === 'classic'
? wrapper.findActiveDrawer().toSelector()
: theme === 'refresh'
? `.${vrDrawerStyles['drawer-content-container']}`
: `.${vrToolbarDrawerStyles['drawer-content-container']}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need new test-utils selectors for these? How customers would scroll the elements in their tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, interesting question, I was thinking about it too.

We don't have any test utils to get scrollable containers except findOptionsContainer in select-alike components that says "Use this element to scroll through the list of options".

Moreover, currently doing page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 }); in customer tests means assuming that the returned element is scrollable. I don't think we support such assumptions.

It seems like a possible, but quite specific test-case where users need that. We could treat it as a feature request, but we haven't heard from customers yet that it's needed

Copy link
Contributor

@orangevolon orangevolon Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this particular PR is blocked by this comment either, but if something is scrollable, I think it's fair to give the customers a way to scroll safely. The theme I've seen so far is that people make feature requests when they get really frustrated by something. Mostly they just use CSS selectors to get to the element they want to scroll.


await page.elementScrollTo(scrollableContainer, { top: 100 });
const resizeHandleAfter = await page.getResizeHandlePosition();
await expect(resizeHandleAfter).toEqual(resizeHandleBefore);
})
Expand Down
15 changes: 13 additions & 2 deletions src/app-layout/__integ__/runtime-drawers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import createWrapper, { AppLayoutWrapper } from '../../../lib/components/test-ut
import { viewports } from './constants';
import { getUrlParams, Theme } from './utils';

import vrDrawerStyles from '../../../lib/components/app-layout/visual-refresh/styles.selectors.js';
import vrToolbarDrawerStyles from '../../../lib/components/app-layout/visual-refresh-toolbar/drawer/styles.selectors.js';

const wrapper = createWrapper().findAppLayout();
const findDrawerById = (wrapper: AppLayoutWrapper, id: string) => {
return wrapper.find(`[data-testid="awsui-app-layout-drawer-${id}"]`);
Expand Down Expand Up @@ -117,7 +120,14 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme
const getScrollPosition = () => page.getBoundingBox('[data-testid="drawer-sticky-header"]');
const scrollBefore = await getScrollPosition();

await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the actual scrollable elements are different in Classic vs VR

const scrollableContainer =
theme === 'classic'
? wrapper.findActiveDrawer().toSelector()
: theme === 'refresh'
? `.${vrDrawerStyles['drawer-content-container']}`
: `.${vrToolbarDrawerStyles['drawer-content-container']}`;

await page.elementScrollTo(scrollableContainer, { top: 100 });
await expect(getScrollPosition()).resolves.toEqual(scrollBefore);
await expect(page.isDisplayed('[data-testid="drawer-sticky-header"]')).resolves.toBe(true);
})
Expand Down Expand Up @@ -316,7 +326,8 @@ describe('Visual refresh toolbar only', () => {
const getScrollPosition = () => page.getBoundingBox('[data-testid="drawer-sticky-header"]');
const scrollBefore = await getScrollPosition();

await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 });
const scrollableContainer = `.${vrToolbarDrawerStyles['drawer-content-container']}`;
await page.elementScrollTo(scrollableContainer, { top: 100 });
await expect(getScrollPosition()).resolves.toEqual(scrollBefore);
await expect(page.isDisplayed('[data-testid="drawer-sticky-header"]')).resolves.toBe(true);
})
Expand Down
2 changes: 1 addition & 1 deletion src/cards/__integ__/sticky-header.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('Cards Sticky Header', () => {
const scrollTopToBtn = '#scroll-to-top-btn';
const toggleStickinessBtn = '#toggle-stickiness-btn';
const toggleVerticalOffsetBtn = '#toggle-vertical-offset-btn';
const overflowParentPageHeight = 300;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it isn't failing, but also doesn't match with actual height https://github.com/cloudscape-design/components/blob/main/pages/cards/sticky-header.page.tsx#L68

const overflowParentPageHeight = 400;
const verticalOffset = 50;

test(
Expand Down
2 changes: 0 additions & 2 deletions src/pie-chart/__integ__/pie-chart.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ describe('Legend', () => {
'can be controlled with mouse',
setupTest(async page => {
// Hover over second segment in the legend
await page.elementScrollTo(legendWrapper.findItems().get(2).toSelector(), { top: 0, left: 0 });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this element

  • isn't scrollable by design
  • is already in the viewport

await page.hoverElement(legendWrapper.findItems().get(2).toSelector());

await expect(page.getText(legendWrapper.findHighlightedItem().toSelector())).resolves.toBe('Chocolate');
Expand Down Expand Up @@ -239,7 +238,6 @@ describe('Legend', () => {
'highlighted legend elements should be not be highlighted when user hovers away',
setupTest(async page => {
// Hover over second segment in the legend
await page.elementScrollTo(legendWrapper.findItems().get(2).toSelector(), { top: 0, left: 0 });
await page.hoverElement(legendWrapper.findItems().get(2).toSelector());

// Verify that no legend is highlighted
Expand Down
2 changes: 0 additions & 2 deletions src/select/__integ__/select.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ test(
const { height: actualDropdownHeight } = await page.getBoundingBox(optionsSelector);
const availableDropdownHeight = smallestContainerHeight - triggerHeight;
expect(actualDropdownHeight).toBeLessThan(availableDropdownHeight);
const { top: containerScrollTop } = await page.getElementScroll('#smallest_container');
expect(containerScrollTop).toBe(0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check was was inherited from v2.1 where it tested the fix for AWSUI-7290. However, the test doesn't work, as the issue is still reproducible in the current version of the components.

I remove the check and a new test should be added once the issue is fixed

})
);

Expand Down
Loading