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

Data grid: Take scrollbar into account #4468

Merged
merged 8 commits into from
Feb 10, 2021

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Feb 1, 2021

Summary

Fixes #4457

I'm surprised it was that easy, but it works fine for the cases I tested (maybe I missed some?). This PR makes sure the vertical scroll bar is taken into account on auto-sizing data grid columns.

To do so, it's simply looking up the clientWidth property of the virtualization container instead of the width of the wrapper container as a reference width. To guard against unforeseen race conditions, it's falling back to the container width if that doesn't work for some reason.

Checklist

- [ ] Check against all themes for compatibility 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 the any docs examples
    - [ ] Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
    -~ [ ] Checked for accessibility including keyboard-only and screenreader modes~
  • A changelog entry exists and is marked appropriately

@flash1293
Copy link
Contributor Author

cc @chandlerprall

@kibanamachine
Copy link

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

@flash1293 flash1293 marked this pull request as ready for review February 3, 2021 13:34
@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

Playing with the examples, found two spots this change doesn't cover:

  • on the main datagrid example, enter full screen and increase pagesize to 100 - both scrollbars appear when only vertical is needed
  • in the virtualization example, switching to Unconstrained height includes both scrollbars instead of only horizontal

@kibanamachine
Copy link

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

@flash1293
Copy link
Contributor Author

flash1293 commented Feb 4, 2021

@chandlerprall OK, this caused some issues :)

To make sure the scrollbar width is always taking into account, I had to specifically re-check the width on page size change, but there is no hook to be sure the dom has relayouted and the scrollbar is visible now, so I have to wait for a fixed number of milliseconds before measuring.

Also, I needed to add a resize observer for the virtualize container specifically to make sure I get notified for all of the changes.

I think I got all the cases and it works, but I'm not really happy with the approach, because it's causing additional re-renders - in most cases it's not visible but it worsens the performance a bit again.

Please take a look and let me know what you think. I can think of one other way how to approach this - instead of measuring the actual virtualized container width, calculating it so we know the right width from the start (rendering the outer container, taking its width and height, checking for current pagination and assumed cell height whether we will get a scrollbar, then subtracting it from the outer container width). It seems very brittle though (because the actual layout can diverge from the calculation due to a number of things, causing the grid to render with the wrong width) and it would be necessary to refactor a lot about the current data_grid / data_grid_body separation.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Thanks @flash1293! Did a functional review. Works as described. Will leave the code review to @chandlerprall.

CHANGELOG.md Outdated Show resolved Hide resolved
chandlerprall and others added 2 commits February 10, 2021 08:43
Co-authored-by: Dave Snider <dave.snider@gmail.com>
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

will merge on passing CI

@kibanamachine
Copy link

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

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

Successfully merging this pull request may close these issues.

[EuiDataGrid] Unnecessary scroll bars when constrained
4 participants