-
Notifications
You must be signed in to change notification settings - Fork 205
fix: prevent list item jump on hover in virtual scrolling (v2) #4064
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4064 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 869 869
Lines 25473 25489 +16
Branches 9209 9215 +6
=======================================
+ Hits 24745 24761 +16
Misses 681 681
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
61dbfeb to
68dbe3d
Compare
SpyZzey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes still have two bugs. In the Autosuggest, select-all still shifts when hovering.
https://d21d5uik3ws71m.cloudfront.net/components/c8deadcd58165722038f814907d79a25e8edca8f/dev-pages-react18/index.html#/light/multiselect/select-all/?virtualScroll=true&closeAfter=false
Also, the checkbox in multiselect is shifting slightly when selecting:
https://d21d5uik3ws71m.cloudfront.net/components/c8deadcd58165722038f814907d79a25e8edca8f/dev-pages-react18/index.html#/light/multiselect/screenshot
Addressed in new revision. |
| &.selected { | ||
| border-width: awsui.$border-divider-list-width; | ||
| padding-block: calc(#{styles.$option-padding-vertical} + #{$virtual-border-offset}); | ||
| padding-inline: calc(#{styles.$control-padding-horizontal} + #{$virtual-border-offset}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we repeat styles from the non-sticky items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We apply these styles to virtual elements, but sticky elements aren't virtual. Perhaps it would be better to merge .virtual and .sticky, but the linter didn't like my first approach. I'll try a different one…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to create a few simple mixins in this file so to share the common styles between sticky and virtual?
fix: inconsistent dropdown styling with virtual scroll refactor: deduplicate code
- Each item is shifted up by its index in pixels - The layout strut height is reduced by the number of items to account for the cumulative overlap
Use virtual window index instead of global index when calculating position offset. This prevents excessive whitespace when scrolling through large lists with virtual scrolling enabled.
4501b63 to
3b628c4
Compare
Note: This PR supersedes PR#3943 and includes an additional fix. The original implementation used
globalIndexfor the position offset calculation, which caused whitespace issues when scrolling through large lists:Changed to use
index(virtual window position) instead ofglobalIndex(absolute list position) to correctly calculate the 1px overlap offset:Description
With virtual scrolling enabled, selectable items would "jump" on hover/selection due to border width changes (AWSUI-61303). Virtual scrolling positions items absolutely, so when border width changes from 1px to 2px on hover, the browser recalculates layout dimensions causing visible position shifts.
Additionally, option groups were missing the separating top border.
The solution uses box-shadows when virtual scrolling is enabled to simulate thick borders while maintaining thin actual borders. An alternative solution of making all borders the same thickness (hover / non-hover) would change the visual appearance compared to how it's supposed to look.
In summary:
How has this been tested?
Changes ran through screenshot test and have been verified locally.
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.