[EuiDataGrid] improve height calculation #5447
Conversation
…o auto fit will expand the grid
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5447/ |
|
Tested on Chrome, Edge, and Safari. Looks good except for one scenario:
Also for some reason my local Firefox for some reason just has never shown grid heights correctly (see #5313 (review)) so you might want someone else to corroborate that it works there. It's showing a bunch of really bizarre extra height/whitespace for me on FF 🤷♀️ |
chandlerprall
changed the title
Pushed a fix, which triggers the grid body to re-render when any rows report a new height. If it looks good, I'll abstract the useState usage out so it's not such a weird block of code in the middle of other logic |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5447/ |
|
@constancecchen I've pushed three change sets:
|
| @@ -254,6 +255,67 @@ export function getParentCellContent(_element: Node | HTMLElement) { | |||
| return element; | |||
| } | |||
|
|
|||
| // computes the unconstrained (total possible) height of a grid | |||
| const useUnconstrainedHeight = ({ | |||
There was a problem hiding this comment.
This entire commit + set of comments is amazing and really helps me understand what this PR is doing. Thanks for taking the time to do it!! 😍
In the future, I might poke a bit more at splitting up even more of data_grid_body's logic into separate utils/hooks like this for organization - I personally think it's super useful for dev experience and unit testing. Thanks for the awesome headstart!
There was a problem hiding this comment.
Breaking out into more hooks would be great; likely a great time to pair as well
There was a problem hiding this comment.
This is seriously super cool. Thanks so much for going the extra mile here, I think it'll really pay off in the future in terms of dev experience and maintainability.
I have one minor non-blocking suggestion, but other than that I've tested this on Chrome, Safari, and Edge and confirm it's all working as expected. My Firefox is sadly still funkytown so if you could test that on my behalf it'd be super appreciated!
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
|
In addition to Constance's 👁️ , I've tested in Chrome, FF, Edge, and Safari |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5447/ |
chandlerprall
merged commit 9d816bf
into
elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher
…5415) * [EuiDataGrid] Toolbar UI layout reorganization (#5334) * [EuiDataGrid] Add extra left prepend position to the `additionalControls` API (#5394) * [EuiDataGrid] Add rowHeightSwitcher logic + API change (#5372) * [EuiDataGrid] Add `onChange` callbacks for display selector changes (#5424) * [EuiDataGrid] Row height switcher should override `rowHeights` + add reset button (#5428) * [EuiDataGrid] improve height calculation (#5447) Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
Summary
Fixes a bug that was uncovered by adding the row height switcher, where going from single or line heights to auto fit wouldn't trigger a resize of the grid, as it is unable to guess the heights of rows when auto. This expands the unconstrained height calculation to include known heights and continue guessing at unknown rows, as auto-fit cells render their heights become known, causing the container to resize.
Added a cypress test for regression. Not sure if the approach grab the original height and do the rest of the test in a
.then()block is the recommended approach, but it works and I couldn't find a way to do the comparison at a single block level.Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] A changelog entry exists and is marked appropriately