[EuiDataGrid] Fix row height not updating dynamically when set to `auto` by cee-chen · Pull Request #5281 · elastic/eui

cee-chen · 2021-10-18T20:46:53Z

Summary

This PR fixes an issue reported by the Discover team when any props (they tested with columns, I ran into this when changing rowHeightOptions dynamically) change on EuiDataGridCell.

It essentially removes several performance APIs in favor of updating the heights cache more aggressively/frequently to get behavior working as expected:

isHeightSame
clearHeightsCache

Before

before.mp4

After

fix.mp4

QA

Go to http://localhost:8031/#/tabular-content/data-grid-row-heights-options#auto-heights-for-rows
Scroll down to the last example on the page
Sort by any column and change sorting
Confirm the auto height continues to work throughout the sorts/changing cell contents

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

~~- [ ] 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-18T21:23:45Z

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

kibanamachine · 2021-10-18T22:45:39Z

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

src/components/datagrid/body/data_grid_body.tsx

- 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

- 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

cee-chen · 2021-10-21T17:30:15Z

src/components/datagrid/body/data_grid_cell.tsx

-    // check if we should update cell because height was changed
-    if (
-      this.cellRef.current &&
-      nextProps.getRowHeight &&
-      nextProps.rowHeightUtils
-    ) {
-      if (
-        !nextProps.rowHeightUtils?.compareHeights(
-          this.cellRef.current.offsetHeight,
-          nextProps.getRowHeight(nextProps.rowIndex)
-        )
-      ) {
-        return true;
-      }
-    }
-


Quick walkthrough of changes:

0eab3ab is the primary workaround @chandlerprall proposed and @VladLasitsa had in his draft PR

(this code diff) a553005 removes height check logic from shouldComponentUpdate as well. I figured if we aren't checking for same height, it makes sense to remove this code also to reduce extra updates. When I tested with auto height I did not see any issues or problems with height not updating when it should (I tested sorting, density, and hiding/reordering columns).

Some optional proposed refactors: 6b870fd

I DRY'd it out so that both the ResizeObserver and props change could call the same method. This leads to some repetition with the height being obtained twice in the observer, but it will not be an issue for long as I'll be refactoring/removing the cell wrapper observer in the row height switcher PR (it does not appear to be doing anything)

I refactored rowHeightUtils.isAutoHeight to accept an undefined rowHeightsOptions

kibanamachine · 2021-10-21T17:39:54Z

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

chandlerprall

Found one logic issue and another change to revert, but all of the examples are working great!

src-docs/src/views/datagrid/row_line_height.tsx

src/components/datagrid/row_height_utils.ts

- 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

kibanamachine · 2021-10-21T19:42:56Z

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

chandlerprall · 2021-10-21T20:13:33Z

Weird unit test outcome, jenkins test this

chandlerprall

Changes LGTM! Let's see if @VladLasitsa finds anything with the Kibana usage 🤞

kibanamachine · 2021-10-21T20:45:45Z

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

…tsOptions changes

kibanamachine · 2021-10-25T16:13:19Z

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

kibanamachine · 2021-10-26T19:52:58Z

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

cee-chen · 2021-10-26T21:40:02Z

I think the last commit (which removes clearHeightCache completely) did the trick - @VladLasitsa can you test again and let us know what you see?

VladLasitsa · 2021-10-27T07:16:05Z

I think the last commit (which removes clearHeightCache completely) did the trick - @VladLasitsa can you test again and let us know what you see?

I see problem: when we hide column cell heights are not recalculating.

chandlerprall · 2021-10-27T13:43:39Z

Created a PR (cee-chen#3) against this branch with a proposed fix to update row height on column removal.

cee-chen · 2021-10-27T15:42:50Z

@chandlerprall Ha, you read my mind - that was almost exactly the approach I was going to try :)

+ add optional isAutoHeight check

chandlerprall

Changes LGTM, tested in many ways and locations

VladLasitsa

LGTM

This reverts commit 17c6eb8.

kibanamachine · 2021-10-27T16:50:02Z

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

kibanamachine · 2021-10-27T18:44:56Z

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

cee-chen · 2021-10-27T18:46:15Z

OK, this time with feeling: one last QA round and if we haven't found anything, I think this should be good to go :)

I tested with sorting, reordering, hiding, density on EUI docs, and with auto on/off on the Kibana deployed instance, and everything was working for me 🤞

VladLasitsa

LGTM! Good job)

…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 <chandler.prall@gmail.com>

cee-chen added bug labels Oct 18, 2021

cee-chen requested review from VladLasitsa and chandlerprall October 18, 2021 20:46

cee-chen force-pushed the datagrid-auto-height-props-update branch from a202fb8 to 07ea516 Compare October 18, 2021 20:52

cee-chen force-pushed the datagrid-auto-height-props-update branch from 07ea516 to 8214083 Compare October 18, 2021 22:19

cee-chen changed the title ~~[EuiDataGrid] Fix incorrect content height check when row height is set to auto~~ [EuiDataGrid] Fix row height not updating dynamically when set to auto Oct 18, 2021

VladLasitsa suggested changes Oct 19, 2021

View reviewed changes

src/components/datagrid/body/data_grid_body.tsx Outdated Show resolved Hide resolved

cee-chen added 4 commits October 21, 2021 10:11

[REVERT ME] Discover issue repro + auto testing

17c6eb8

Remove isHeightSame check

0eab3ab

Remove same height check from shouldComponentUpdate

a553005

- 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

cee-chen force-pushed the datagrid-auto-height-props-update branch from 9e7feb2 to c91cad5 Compare October 21, 2021 17:12

cee-chen added 2 commits October 21, 2021 10:14

Improve unit tests

10317cb

Add changelog entry

9eb6651

cee-chen force-pushed the datagrid-auto-height-props-update branch from c91cad5 to 9eb6651 Compare October 21, 2021 17:15

cee-chen commented Oct 21, 2021

View reviewed changes

chandlerprall suggested changes Oct 21, 2021

View reviewed changes

src-docs/src/views/datagrid/row_line_height.tsx Outdated Show resolved Hide resolved

src/components/datagrid/row_height_utils.ts Show resolved Hide resolved

[PR feedback] Fix isAutoHeight false positive

9e844ea

- 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

chandlerprall approved these changes Oct 21, 2021

View reviewed changes

This was referenced Oct 22, 2021

[WIP][DataTable] Remove temp fix fro EuiDataGrid elastic/kibana#116036

Closed

[Lens] Auto height setting for table elastic/kibana#115582

Closed

[PR feedback/Discover testing] Trigger component update when rowHeigh…

22aed7a

…tsOptions changes

cee-chen force-pushed the datagrid-auto-height-props-update branch from 970e4b9 to 67d27af Compare October 26, 2021 19:20

chandlerprall mentioned this pull request Oct 27, 2021

Unset row+column height values on unmount cee-chen/eui#3

Merged

chandlerprall mentioned this pull request Oct 27, 2021

Fix clearing heights cache behavior in datagrid #5291

Closed

chandlerprall and others added 3 commits October 27, 2021 08:44

Unset row+column height values on unmount (#3)

03e91d2

Merge branch 'master' into datagrid-auto-height-props-update

6382ca8

Add unit test for new unsetRowHeight

bd2e315

+ add optional isAutoHeight check

cee-chen requested a review from chandlerprall October 27, 2021 15:58

chandlerprall approved these changes Oct 27, 2021

View reviewed changes

VladLasitsa approved these changes Oct 27, 2021

View reviewed changes

Constance and others added 2 commits October 27, 2021 09:24

Merge branch 'master' into datagrid-auto-height-props-update

ed76e3b

Revert "[REVERT ME] Discover issue repro + auto testing"

83f7eea

This reverts commit 17c6eb8.

cee-chen enabled auto-merge (squash) October 27, 2021 16:26

cee-chen disabled auto-merge October 27, 2021 16:54

chandlerprall added 2 commits October 27, 2021 12:15

Remove hidden columns from row height cache

255ae54

Early return in getRowHeight

4b66cbf

cee-chen requested a review from VladLasitsa October 27, 2021 18:46

VladLasitsa approved these changes Oct 28, 2021

View reviewed changes

cee-chen merged commit 8563fbe into elastic:master Oct 28, 2021

cee-chen deleted the datagrid-auto-height-props-update branch October 28, 2021 15:23

This was referenced Oct 28, 2021

Upgrade EUI to v39.1.2 elastic/kibana#116690

Merged

Upgrade to EUI v40.1.0 elastic/kibana#116692

Merged

Conversation

cee-chen commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Before

After

QA

Checklist

Uh oh!

kibanamachine commented Oct 18, 2021

Uh oh!

kibanamachine commented Oct 18, 2021

Uh oh!

Uh oh!

cee-chen Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 21, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Oct 21, 2021

Uh oh!

chandlerprall commented Oct 21, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 21, 2021

Uh oh!

kibanamachine commented Oct 25, 2021

Uh oh!

kibanamachine commented Oct 26, 2021

Uh oh!

cee-chen commented Oct 26, 2021

Uh oh!

VladLasitsa commented Oct 27, 2021

Uh oh!

chandlerprall commented Oct 27, 2021

Uh oh!

cee-chen commented Oct 27, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

VladLasitsa left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 27, 2021

Uh oh!

kibanamachine commented Oct 27, 2021

Uh oh!

cee-chen commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladLasitsa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cee-chen commented Oct 18, 2021 •

edited

Loading

cee-chen Oct 21, 2021 •

edited

Loading

cee-chen commented Oct 27, 2021 •

edited

Loading