Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: SC-59775 fix nested select height after long render #1084

Merged
merged 4 commits into from
Oct 25, 2024
Merged

fix: SC-59775 fix nested select height after long render #1084

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
469b932
fix: SC-59775 fix nested select height after long render
0ces Oct 21, 2024
39a5a84
chore: add storybook
0ces Oct 23, 2024
f1b02b4
fix: Toggle extra column in useGrowingTextField.stories.tsx
0ces Oct 25, 2024
48cc4ec
fix: PR feedback
0ces Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/inputs/hooks/useGrowingTextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0ces just curious, but is this only an issue in virtualized tables, or just any GridTable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@blambillotte blambillotte Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

setTimeout(() => onHeightChange(), 200);
}
}, [onHeightChange, value, inputRef, disabled, inputWrapRef]);
}
Loading