chore: Fix elements scrolling with elementScrollTo in integration tests #3138
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3138 +/- ##
========================================
Coverage 96.44% 96.44%
========================================
Files 790 790
Lines 22249 22249
Branches 7231 7637 +406
========================================
Hits 21458 21458
+ Misses 784 739 -45
- Partials 7 52 +45 ☔ View full report in Codecov by Sentry. |
Al-Dani
force-pushed
the
non-scrollable-tests
branch
from
356e3c5 to
28c6984
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Al-Dani
force-pushed
the
non-scrollable-tests
branch
from
5286957 to
6ab9eea
Compare
Al-Dani
force-pushed
the
non-scrollable-tests
branch
from
0c078c3 to
ae9cd42
Compare
| @@ -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 }); | |||
There was a problem hiding this comment.
this element
- isn't scrollable by design
- is already in the viewport
| @@ -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); | |||
There was a problem hiding this comment.
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
| @@ -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 }); | |||
There was a problem hiding this comment.
the actual scrollable elements are different in Classic vs VR
Al-Dani
force-pushed
the
non-scrollable-tests
branch
from
ae9cd42 to
9b483f1
Compare
| ? wrapper.findActiveDrawer().toSelector() | ||
| : theme === 'refresh' | ||
| ? `.${vrDrawerStyles['drawer-content-container']}` | ||
| : `.${vrToolbarDrawerStyles['drawer-content-container']}`; |
There was a problem hiding this comment.
Do we need new test-utils selectors for these? How customers would scroll the elements in their tests?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Description
Related links, issue #, if available:
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.