-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiDataGrid] Reduce layout shifts when auto-sizing overscan rows #6028
Conversation
06ac5bb
to
016cf98
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/ |
ℹ️ This is being split up into smaller PRs: |
I think what's remaining in this PR without the imperative ref and row height tests work is probably ok for a single PR! But if you find another natural way to conceptually split the remaining work, I'm all for more atomic PRs! |
I think I might submit the functionality with a simplified example first and maybe create a more complex example later. |
🗒️ Summary
This adds a mechanism to the
EuiDataGrid
to compensate for vertical layout shifts when rows above the currently visible ones change their height. This often happens when the row heights are"auto"
-measured and new rows are rendered in the overscan area. By specifying ascrollAnchorRow: 'start'
, the grid will try to scroll to keep the vertical position of the topmost visible row stable in the viewport. This behavior is opt-in.This also exposes the
scrollTo()
andscrollToItem
methods on the imperative API of the grid component.closes #6024
🎨 Previews
Scrolling without scroll anchoring - observe how the indices jump as the new rows get measured
eui-data-grid-without-scroll-anchoring-2022-07-06-20-00.mp4
Scrolling with scroll anchoring - observe how the indices stay consistent as the new rows get measured
eui-data-grid-with-scroll-anchoring-2022-07-06-19-59.mp4
🕵️♀️ Review notes
RowHeightUtils
receive references to various dependencies. It now receives them asMutableRefObject
s in its constructor without the need for several imperativeset*
methods.RowHeightUtils
were not working as intended, because they used some jest features incorrectly. I tried to correct for the following problems before adding additional test cases for the newcompensateForLayoutShift
method:jest.useFakeTimers
was called in several places directly indescribe
blocks, which means they're not bound to the lifecycle of the test cases. As a consequence some tests failed unpredictably depending on the execution order of thedescribe
blocks. Some test cases also were not callingjest.runAllTimers
, which caused severalsetTimeouts
not to be executed and lead to several false positive test results.jest.spyOn
was also called indescribe
blocks directly, but their implementation was never restored. That means that some spied-upon function implementations would be replaced by no-ops when the mocks were reset, which in turn lead to false positives and negatives.📋 Checklist
Updated the Figma library counterpart