fix: SC-59775 fix nested select height after long render #1084
Conversation
|
@0ces since we don't have a unit test for this, can you record a screenshare of the before/after behavior? Might help maintainers know how to not break this going forward, if they |
| @@ -42,6 +42,8 @@ export function useGrowingTextField({ inputRef, inputWrapRef, value, disabled }: | |||
| return; | |||
| } | |||
| onHeightChange(); | |||
| // This is a hack to re-measure the height of the textarea after all the content has been rendered (i.e. after multiple GridTable children have been rendered) | |||
There was a problem hiding this comment.
@0ces just curious, but is this only an issue in virtualized tables, or just any GridTable?
There was a problem hiding this comment.
@stephenh I think only in virtualized tables, it happens when the columns change (remove/hide a column) and the tables changes from not grouped to grouped, I'll have a PR that solves the bug in our front-end by not changing the column when the table gets grouped.
There was a problem hiding this comment.
@0ces so I'm not following if (when a column is hidden/changed from grouped to not grouped):
a) this useLayoutEffect is actually triggered by a dirty deps change (so we almost work), but because of "reasons related to virtualization", the measurement isn't correct, and we need to try again, or
b) this useLayoutEffect just isn't triggered at all when column is hidden/grouped, but the 200ms from the original/only invocation happens to catch it.
I'm thinking surely it's not b), because 200ms is so small...
Can you confirm a) vs. b)?
Fwiw my musing is that, because of virtualization, react-virtuoso (or GridTable) is being "smart" and not even causing our cell to get re-rendered?...
@blambillotte do you understand how/why this fix works?
There was a problem hiding this comment.
Interesting 🤔 I wonder if the check on line 40 is allowing the similar code (with the same setTimeout hack) on line 41 to run or not - I sort of assume the added time has no real impact vs just moving out of the sync call stack due to the setTimeout.
I can pull this down and poke a bit
There was a problem hiding this comment.
@stephenh this bug it's "hard" to reproduce, I think option b is the closest to what I've seen, but when it happens it is consistent, when trying to debug these it looked like 200ms was just enough for the onHeightChange to read the real final height and scroll states of the refs. I'll try to add a test that reproduces this.
There was a problem hiding this comment.
Nice! Really appreciate the storybook!
Maybe update this in-source comment to just include "See the InVirtualizedTable storybook for reproducing this behavior", so maintainers know exactly where to go to futz with this line/setTimeout.
## [2.369.7](v2.369.6...v2.369.7) (2024-10-25) ### Bug Fixes * SC-59775 fix nested select height after long render ([#1084](#1084)) ([4ef1241](4ef1241))
|
🎉 This PR is included in version 2.369.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Issue: when rendering a SelectField in a GridTable with no nested rows the select renders with the correct height, when updating the table to have nested rows the select field will fail calculating the new correct height, this change adds another delayed called that gives a better chance of having enough time for GridTable finishing the rendering so that
useGrowingTextFieldwill get the correct value.