[EuiDataGrid] Fix row height changes from the rowHeightsOptions prop not correctly triggering grid rerenders#5712
Conversation
- move rowHeightUtils.setRerenderGridBody to same location as rowHeightUtils.setGrid - move inline comment to where fn is being invoked - update useRowHeightUtils hook comment
- not totally sure why this requestAnimationFrame is causing issues but not others
| // Mock requestAnimationFrame to run immediately | ||
| jest | ||
| .spyOn(window, 'requestAnimationFrame') | ||
| .mockImplementation((cb: any) => cb()); |
There was a problem hiding this comment.
Not totally sure what other options we have, but without this we get a bunch of React async errors on data_grid.test.tsx 😕 (same for setTimeout as well, I tried both)
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5712/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5712/ |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
LGTM! Doesn't feel too sledge-hammery, and could help catch similar things slipping by.
Tested this by bringing the datagrid.js example changes locally to confirm that setup recreates the issue, then tested the published example and saw no extra lines hanging out.
|
Sweet! I'll probably end up writing unit tests for |
…ng` to undefined - rather than just omitting it - see elastic#5713 (comment)
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5712/ |
3 participants
Summary
This PR closes #5700, which is
rowHeightsOptionsprops changes not correctly triggering body rerenders (and thus leaving stale scrollbar artifacts per the linked issue).I recommend following along by commit, as I also added some incidental code cleanup & comments in the 2nd commit.
@chandlerprall, I'm not sure if this approach is too much of a sledgehammer vs a more nuanced way of calling
rerenderGridBody- feel free to shout if you have any other ideas.Before
This issue was occuring for both
lineCountand staticheightchanges, but not auto height because auto height rerenders the grid body when it sets the height cache.You can see on the first change that the grid height/width is not correctly changing (based on scroll bars not appearing/disappearing when they should), but subsequent rerenders do cause the correct appearance.
After
Checklist
I left out Jest tests for now as we don't currently have any for
useRowHeightUtils, but would be open to adding them as part of this PR if we think this approach is fine.