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.

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

Closed
wants to merge 10 commits into from

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jul 5, 2022

🗒️ 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 a scrollAnchorRow: '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() and scrollToItem 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

  • I refactored the way the RowHeightUtils receive references to various dependencies. It now receives them as MutableRefObjects in its constructor without the need for several imperative set* methods.
  • The tests for the 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 new compensateForLayoutShift method:
    • jest.useFakeTimers was called in several places directly in describe 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 the describe blocks. Some test cases also were not calling jest.runAllTimers, which caused several setTimeouts not to be executed and lead to several false positive test results.
    • jest.spyOn was also called in describe 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

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@weltenwort weltenwort self-assigned this Jul 5, 2022
@weltenwort weltenwort force-pushed the data-grid-scroll-anchoring branch from 06ac5bb to 016cf98 Compare July 6, 2022 14:26
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6028/

@weltenwort weltenwort marked this pull request as ready for review July 6, 2022 19:15
@chandlerprall chandlerprall requested a review from cee-chen July 6, 2022 19:41
@cee-chen cee-chen requested a review from chandlerprall July 6, 2022 20:07
@weltenwort
Copy link
Member Author

ℹ️ This is being split up into smaller PRs:

@weltenwort weltenwort closed this Jul 19, 2022
@cee-chen
Copy link
Contributor

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!

@weltenwort
Copy link
Member Author

I think I might submit the functionality with a simplified example first and maybe create a more complex example later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Reduce layout shift when row heights are measured automatically
3 participants