[EuiDataGrid] Fix incorrect full screen height #5557
Conversation
- There's no need for a separate isFullScreen branch in this logic: EuiDataGrid's wrapper will update its dimensions on full screen toggle, and react-window should simply use that wrapper's height & width, the same way it does in non-full-screen mode
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5557/ |
|
Huh, so.... that Cypress spec failure is legit, I can repro it in https://eui.elastic.co/pr_5557/#/tabular-content/data-grid-focus.
Basically only when headers are not interactive, I can add the toolbar ref back in if we want it, but I could also fix the issue with a forceRender... the issue just seems super bizarre though honestly, would def appreciate jumping on a call to debug it if you have time. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5557/ |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
Woohoo! Love seeing less code leading to a better experience. This shows how strong the dimensions watching/height calculation has become.
I played with the examples locally and couldn't find any bugs/regressions. Constance and I paired to look at why non-interactive headers required a workaround, which led to acf5853
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5557/ |
* Fix incorrect scrolling height on full screen grids - There's no need for a separate isFullScreen branch in this logic: EuiDataGrid's wrapper will update its dimensions on full screen toggle, and react-window should simply use that wrapper's height & width, the same way it does in non-full-screen mode * Remove unused props * Remove now-unnecessary isFullScreen prop from EuiDataGridBody * Remove now unnecessary toolbar ref/height observer * Add changelog entry * Workaround for failing spec / toolbar hiding itself on non-interactive headers * Swap a useRef for a useState to trigger a re-render after mount Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
3 participants
Summary
closes #5139
closes #5131
You gotta love it when a fix just involves removing logic! 🔥
Quick summary of changes - the previous
finalHeight = window.innerHeight - toolbarHeight - headerRowHeight - footerRowHeightcalculation likely came from pre-virtualization and is no longer necessary. There's no need for a separateisFullScreenlogic - EuiDataGridBody's wrapper dimensions update on full screen toggle and resize correctly due to flexbox.react-windowshould simply use that wrapper's height & width, the same way it does in non-full-screen mode.Before
Note how bottom row is cut off and how the horizontal scrollbar does not appear after increasing the width of a column
After
Note how bottom row is not cut off and how the horizontal scrollbar does appear after increasing the width of a column
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examplesAdded or updated jest and cypress testsNOTE: No jest tests added here due to no unit tests previously existing for this file + the presence ofIS_JEST_ENVIRONMENT, but it's on my TODO list to add more unit tests for ourutils/shortly- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes