-
Notifications
You must be signed in to change notification settings - Fork 209
fix: Fixes trembling of table columns when resizing #4159
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.
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4159 +/- ##
=======================================
Coverage 97.14% 97.15%
=======================================
Files 873 874 +1
Lines 25642 25655 +13
Branches 9284 9283 -1
=======================================
+ Hits 24910 24924 +14
+ Misses 726 725 -1
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/table/resizer/index.tsx
Outdated
| if (!elements) { | ||
| return; | ||
| } | ||
| const resizeColumn = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is called on every pointer move - so throttling helps by ignoring intermediate updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] I checked our codebase and notice multiple instance using throttle with useMemo/useCallback. I'd suggest to introduce an abstraction useThrottle for them. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I added useThrottleCallback (similar to existing useDebounceCallback).
| // Using the tableRef getBoundingClientRect().width instead of the theadRef because in VR | ||
| // the tableRef adds extra padding to the table and by default the theadRef will have a width | ||
| // without the padding and will make the sticky header width incorrect. | ||
| secondaryTableRef.current.style.inlineSize = `${tableRef.current.getBoundingClientRect().width}px`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table column width utils set widths of both normal and sticky headers at the same time, so there is no need to sync table widths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table column width utils set widths of both normal and sticky headers at the same time
Could you point out to the code that does that and why did you change it in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly! Here is where the col widths are synchronised: https://github.com/cloudscape-design/components/blob/main/src/table/use-column-widths.tsx#L141
When researching columns trembling, I realised that the large part of it was due to this line of code (only when sticky header is on). Here we measure the total size of the real table and set it to the sticky table. Due to the async nature of the code, this measurement is not that precise - and often happens before the column widths are settled - and thus contributing to the trembling. I tried to optimise it first, but then realise that this piece of code seems to be no longer relevant, as the same explicit sizes are used for all columns anyways.
NathanZlion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This fixes a table headers trembling when resizing. The issue was reproducible on complex tables and especially those with sticky headers.
Before without sticky header:
Screen.Recording.2026-01-07.at.10.47.42.mov
Before with sticky header:
Screen.Recording.2026-01-07.at.10.48.36.mov
After:
Screen.Recording.2026-01-07.at.10.49.53.mov
Rel: AWSUI-61443
Resolves: #4010
How has this been tested?
The fix can be tested manually on any table with resizable columns but especially those with lots of content. It is worth testing it in both React 16 and 18 and also ensure the sticky headers column sync works fine in both resizable and not resizable tables.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.