[EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init #5284

cee-chen · 2021-10-18T22:58:26Z

Summary

This PR ended up being a more difficult problem to solve than anticipated, and took several twists and turns - see this comment for the most recent description

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 any docs examples~~
- [ ] Added or updated jest and cypress tests NB: Jest tests have yet to be written for row_height_utils, but it's on my radar
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~

A changelog entry exists and is marked appropriately

kibanamachine · 2021-10-18T23:23:19Z

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

chandlerprall · 2021-10-19T18:27:17Z

Looks like this change breaks the Setting a default height and line height for rows example.

I think this is a race condition in FF+Safari between the css stylesheets being added to the document and this test. Navigating to the auto-height example appears to always work, and when I added

if (allStyles.paddingTop === '0px') debugger;

to test with, FF will hit the debugger on a hard refresh, but by the time I can debug the allStyles.paddingTop value in the console it is correctly set at 6px

cee-chen · 2021-10-19T19:51:15Z

Doh, I was wondering what was going on there (but thought it was some weirdness with my computer 🙈)

Race condition makes a ton of sense. Is there any way to wait to run this code until after stylesheets have loaded?

cee-chen · 2021-10-19T19:52:26Z

Or alternatively, can we avoid using CSS for the padding calculations? For example, if we know the grid density, can we just hard code a map of padding heights (which we could later obtain from our CSS-in-JS work)?

cee-chen · 2021-10-21T22:35:40Z

The static map works really well, except unfortunately for lineHeight which is specifically used by calculateHeightForLineCount. The issue is that our base line height is not the same between our Amsterdam and Legacy theme, so the calculation works for one theme but not the other 😢

I have an idea for that issue that I'd like to pitch: when setting lineCount, the cell content actually already knows what height it should be when using due to our usage of CSS line-clamp. So, why not have lineCount simply take advantage of the cell content observer + height cache that we added to rowHeightUtils and use the same height logic as auto, instead of trying to calculate the height for lineCount manually?

Alternatively, if we don't anticipate DataGrid being used in the legacy theme, or if we're deprecating the legacy theme soon, we could just leave this as-is?...

kibanamachine · 2021-10-21T22:59:27Z

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

- lineCount in elastic#5284 will shortly be using the row height cache in addition to auto, so I'm making the fn name less specific again

cee-chen · 2021-10-25T20:37:39Z

If converting this PR to have lineCount use the same height cache that auto does, I'll need #5281 to merge in first - going to convert this PR back to a draft in the meanwhile and pull out the first density font size fix to a separate PR.

Edit: I ended up exploring this but had to find a different solution as CSS line-clamp limits lines to a certain #, but does not expand the row height to that # of lines (sadly), which means if you had less content than the # of lines, the lineCount height was not working as it functioned previously.

- lineCount in elastic#5284 will shortly be using the row height cache in addition to auto, so I'm making the fn name less specific again

…to` (#5281) * Remove isHeightSame check * Remove same height check from shouldComponentUpdate - if we're not checking for sameHeight in componentDidUpdate, we also likely don't need this anymore, and all dynamic user changes appear to work without this code * [Proposed refactors] - Rename `recalculateRowHeight` to `setAutoRowHeight` - DRY out setAutoRowHeight to be reusable by both the resize observer and during props update (NB: This leads to some repetition with the height being obtained twice, but will not be an issue shortly as I'll be refactoring/removing the cell wrapper observer in a future PR) - Refactor rowHeightUtils.isAutoHeight to accept an undefined rowHeightsOptions * Improve unit tests * Add changelog entry * [PR feedback] Fix `isAutoHeight` false positive - occurs if the `defaultHeight` is auto but a specific `rowHeights` line override is not auto TODO: Write a unit test for this in the future * [PR feedback/Discover testing] Trigger component update when rowHeightsOptions changes * [cleanup] Remove now-unused compareHeights util + clean up mock rowHeightUtils as well * [revert] rename setAutoRowHeight back to recalculateRowHeight - lineCount in #5284 will shortly be using the row height cache in addition to auto, so I'm making the fn name less specific again * Add enqueue/timeout to recalculateRowHeight updates - to avoid race conditions with cache being cleared * Remove clearHeightsCache completely * Unset row+column height values on unmount (#3) * Add unit test for new unsetRowHeight + add optional isAutoHeight check * Remove hidden columns from row height cache * Early return in getRowHeight Co-authored-by: Chandler Prall <[email protected]>

…to` (elastic#5281) * Remove isHeightSame check * Remove same height check from shouldComponentUpdate - if we're not checking for sameHeight in componentDidUpdate, we also likely don't need this anymore, and all dynamic user changes appear to work without this code * [Proposed refactors] - Rename `recalculateRowHeight` to `setAutoRowHeight` - DRY out setAutoRowHeight to be reusable by both the resize observer and during props update (NB: This leads to some repetition with the height being obtained twice, but will not be an issue shortly as I'll be refactoring/removing the cell wrapper observer in a future PR) - Refactor rowHeightUtils.isAutoHeight to accept an undefined rowHeightsOptions * Improve unit tests * Add changelog entry * [PR feedback] Fix `isAutoHeight` false positive - occurs if the `defaultHeight` is auto but a specific `rowHeights` line override is not auto TODO: Write a unit test for this in the future * [PR feedback/Discover testing] Trigger component update when rowHeightsOptions changes * [cleanup] Remove now-unused compareHeights util + clean up mock rowHeightUtils as well * [revert] rename setAutoRowHeight back to recalculateRowHeight - lineCount in elastic#5284 will shortly be using the row height cache in addition to auto, so I'm making the fn name less specific again * Add enqueue/timeout to recalculateRowHeight updates - to avoid race conditions with cache being cleared * Remove clearHeightsCache completely * Unset row+column height values on unmount (elastic#3) * Add unit test for new unsetRowHeight + add optional isAutoHeight check * Remove hidden columns from row height cache * Early return in getRowHeight Co-authored-by: Chandler Prall <[email protected]>

NB: Saving this commit for historical purposes; but the issue with this solution is that lineHeight & fontSize change between legacy and Amsterdam theme and thus the calculations are wrong on the legacy theme - Switch to static padding/fontSize/lineHeights maps instead of classes - Undefined line height will NaN without euiDataGridRow CSS - Remove this.fakeCell class instance since it's only used in one place

- Export single default/initial row height const which data_grid_body imports, instead of declaring 2 slightly different heights in 2 different locations - fix rowIndex check that would fail on a rowIndex of 0

…nto a getRowHeightOption helper - this helper DRYs out returning either a specific row's height config or the defaultHeight - we had this logic essentially repeated in multiple places

- Pull out shared/same props into a single obj instead of repeating it 3x & making it less clear which props are different depending on the cell type - remove getRowHeight fn from being passed down; it's not being used - pass setRowHeight only to first column as a performance optimization; this will be used for lineCount heights and since lineCount heights are all the same, we really only need to run it once - clean up footer row cells as well while we're here

…_grid_body - IIRC this is what the state was originally used for; might as well go back to using it for lineCount

- Those were only being used for lineCount calculations; otherwise only padding is needed + remove numberFromPx in favor of a map with existing numbers

…les & remove getComputedCellStyles - re: getComputedCellStyles - I'm not seeing this affect either numeric, lineCount, or auto heights when changing grid density, hence me removing this

Now that we aren't using this CSS for computed styles any longer, we can remove them

- we're exclusively watching the cell content now for both auto and line count heights

- to make it more specific and clearer from the fn name what row height behavior this belongs to

- add unit tests for recalculateLineCountHeight, - mockRowHeightUtils - allow some generic methods to return their actual fns for expedience - Remove jest.mock from data_grid.test.tsx - the __mocks__ folder automatically mocks it - update snapshots/returns accordingly

cee-chen · 2021-10-29T14:44:08Z

jenkins test this

cee-chen

This PR ended up being a whole lot of cleanup mixed in with the actual functionality fixes 🙈 I recommend following along by commit (the cleanups in particular I tried to separate out into their own commits), but I'll also attempt to walk through the actual functionality changes/fixes below!

cee-chen · 2021-10-29T14:44:25Z

src/components/datagrid/row_height_utils.ts

+// TODO: Once JS variables are available, use them here instead of hard-coded maps
+const cellPaddingsMap: Record<EuiDataGridStyleCellPaddings, number> = {
+  s: 4,
+  m: 6,
+  l: 8,
 };


This static padding map is what solves the issue for auto height rows:

Before

After

cee-chen · 2021-10-29T14:56:23Z

src/components/datagrid/row_height_utils.ts

+  calculateHeightForLineCount(cellRef: HTMLElement, lineCount: number) {
+    const computedStyles = window.getComputedStyle(cellRef, null);
+    const lineHeight = parseInt(computedStyles.lineHeight, 10) || 24;
+
    return Math.ceil(
-      lineCount * this.styles.lineHeight +
+      lineCount * lineHeight +
        this.styles.paddingTop +
        this.styles.paddingBottom
    );


This change here (getting the computed line-height from the cell rather than on grid init) is what makes the lineCount height calculation work as before. In particular, the key to solving the 'styles not available on load' bug is that calculateHeightForLineCount gets called by the cell content resizeObserver, which means that if styles load after grid init, the resizeObserver fires and row height will get calculated & updated after. This also applies to/works for density changes!

The full commit is here for reference: 905031a

Here's an example of this working locally on a demo with a lineCount: 10:

lineCount.mp4

I should also mention the way lineCount cells now get and set their height also is from the minRowHeight/setRowHeights state set in data_grid_body.tsx:

eui/src/components/datagrid/body/data_grid_body.tsx

Line 507 in 51a3d2d

const [minRowHeight, setRowHeight] = useState(DEFAULT_ROW_HEIGHT);

I believe this was also originally the way it worked previously on datagrid's first inception, so what's old becomes new, etc. 😅 (but I was able to clean up the cell wrapper observer so we're now down to just 1 observer!)

kibanamachine · 2021-11-01T21:17:03Z

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

cee-chen · 2021-11-02T15:54:50Z

FYI, once this PR lands, I have a follow-up PR for right after it that does some minor organization/clean up of row_height_utils.ts and adds unit tests! 🧹

chandlerprall

Changes LGreatTM! Lots of good refactoring into more followable code paths, couple extra bug fixes, solves the loading issue, etc. 🎉

Tested the published changes across Chrome, Firefox, and Safari, saw no issues with intermittent loading, flash of content or styles, virtualization, or performance.

cee-chen · 2021-11-03T15:37:25Z

Wahoo, thanks Chandler! 🎉

kibanamachine · 2021-11-03T16:07:07Z

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

kibanamachine · 2021-11-03T16:53:56Z

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

cee-chen requested a review from chandlerprall October 18, 2021 22:58

cee-chen added the bug label Oct 18, 2021

cee-chen force-pushed the datagrid-computed-styles branch from 7c440a0 to 3e833d3 Compare October 21, 2021 22:29

cee-chen marked this pull request as draft October 25, 2021 20:38

cee-chen force-pushed the datagrid-computed-styles branch from c359430 to 8cbe865 Compare October 28, 2021 19:45

cee-chen added 3 commits October 29, 2021 05:04

Misc rowHeightUtils fixes/refactors

eed7252

- Export single default/initial row height const which data_grid_body imports, instead of declaring 2 slightly different heights in 2 different locations - fix rowIndex check that would fail on a rowIndex of 0

Refactor isDefinedHeight and several rowHeightsOptions logic checks i…

aa856e6

…nto a getRowHeightOption helper - this helper DRYs out returning either a specific row's height config or the defaultHeight - we had this logic essentially repeated in multiple places

cee-chen force-pushed the datagrid-computed-styles branch from 2655093 to 2d9edf4 Compare October 29, 2021 13:04

cee-chen added 9 commits October 29, 2021 07:07

Convert lineCount rows to use this.props.setRowHeight state from data…

905031a

…_grid_body - IIRC this is what the state was originally used for; might as well go back to using it for lineCount

Update computeStylesForGridCell to remove fontSize & lineHeight calcs

326e4e4

- Those were only being used for lineCount calculations; otherwise only padding is needed + remove numberFromPx in favor of a map with existing numbers

Rename computeStylesForGridCell now that it's no longer computing sty…

51a3d2d

…les & remove getComputedCellStyles - re: getComputedCellStyles - I'm not seeing this affect either numeric, lineCount, or auto heights when changing grid density, hence me removing this

Remove unused CSS classes

67ba739

Now that we aren't using this CSS for computed styles any longer, we can remove them

Remove now-unused cell wrapper observer

c3602b8

- we're exclusively watching the cell content now for both auto and line count heights

Rename recalculateRowHeight to recalculateAutoHeight

819c3fa

- to make it more specific and clearer from the fn name what row height behavior this belongs to

Add changelog entry

e4a77cf

cee-chen force-pushed the datagrid-computed-styles branch from 2d9edf4 to e4a77cf Compare October 29, 2021 14:08

cee-chen marked this pull request as ready for review October 29, 2021 14:23

cee-chen changed the title ~~[EuiDataGrid] Fix Firefox & Safari not correctly calculating cell heights based on computed styles~~ [EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init Oct 29, 2021

cee-chen commented Oct 29, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

cee-chen added 2 commits November 1, 2021 13:45

Fix crashing eui.d.ts

cb0b118

Merge branch 'master' into datagrid-computed-styles

bb1da41

chandlerprall approved these changes Nov 2, 2021

View reviewed changes

cee-chen added 2 commits November 3, 2021 08:35

Merge branch 'master' into datagrid-computed-styles

03dd37d

Fix changelog after release

edeaa91

cee-chen enabled auto-merge (squash) November 3, 2021 15:37

Merge branch 'master' into datagrid-computed-styles

0d1eb7e

cee-chen merged commit 95e15fa into elastic:master Nov 3, 2021

cee-chen deleted the datagrid-computed-styles branch November 3, 2021 17:01

1Copenut mentioned this pull request Nov 19, 2021

Bumping EUI to v41.2.2 elastic/kibana#119205

Merged

flash1293 mentioned this pull request Dec 13, 2021

Table visualization tables not visible when "split table" is used elastic/kibana#119456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init #5284

[EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init #5284

cee-chen commented Oct 18, 2021 •

edited

Loading

kibanamachine commented Oct 18, 2021

chandlerprall commented Oct 19, 2021

cee-chen commented Oct 19, 2021

cee-chen commented Oct 19, 2021

cee-chen commented Oct 21, 2021 •

edited

Loading

kibanamachine commented Oct 21, 2021

cee-chen commented Oct 25, 2021 •

edited

Loading

cee-chen commented Oct 29, 2021

cee-chen left a comment

cee-chen Oct 29, 2021

cee-chen Oct 29, 2021 •

edited

Loading

cee-chen Oct 29, 2021 •

edited

Loading

This comment has been minimized.

kibanamachine commented Nov 1, 2021

cee-chen commented Nov 2, 2021

chandlerprall left a comment

cee-chen commented Nov 3, 2021

kibanamachine commented Nov 3, 2021

kibanamachine commented Nov 3, 2021

[EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init #5284

[EuiDataGrid] Refactor row height calculations to no longer need computed styles on grid init #5284

Conversation

cee-chen commented Oct 18, 2021 • edited Loading

Summary

Checklist

kibanamachine commented Oct 18, 2021

chandlerprall commented Oct 19, 2021

cee-chen commented Oct 19, 2021

cee-chen commented Oct 19, 2021

cee-chen commented Oct 21, 2021 • edited Loading

kibanamachine commented Oct 21, 2021

cee-chen commented Oct 25, 2021 • edited Loading

cee-chen commented Oct 29, 2021

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen Oct 29, 2021

Choose a reason for hiding this comment

Before

After

cee-chen Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

kibanamachine commented Nov 1, 2021

cee-chen commented Nov 2, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 3, 2021

kibanamachine commented Nov 3, 2021

kibanamachine commented Nov 3, 2021

cee-chen commented Oct 18, 2021 •

edited

Loading

cee-chen commented Oct 21, 2021 •

edited

Loading

cee-chen commented Oct 25, 2021 •

edited

Loading

cee-chen Oct 29, 2021 •

edited

Loading

cee-chen Oct 29, 2021 •

edited

Loading