-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Update column width calculation logic #3747
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3747 +/- ##
==========================================
- Coverage 98.80% 98.69% -0.12%
==========================================
Files 47 47
Lines 3424 3436 +12
Branches 743 750 +7
==========================================
+ Hits 3383 3391 +8
- Misses 41 45 +4
🚀 New features to boost your workflow:
|
src/utils/index.ts
Outdated
|
|
||
| export function clampColumnWidth<R, SR>( | ||
| width: number, | ||
| export function getColumnWidthForMeasurement<R, SR>( |
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.
I believe that this function is not needed at all if we pass min-width and max-width to columns styles; then setting any width will not break the grid layout
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 is needed to fix this bug. We already set min/max width on the measuring cell
| let width = getColumnWidth(column); | ||
|
|
||
| if (typeof width === 'number') { | ||
| width = clampColumnWidth(width, column); |
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.
It is possible that a column has a width less than minWidth or greater than maxWidth so we may have more or less viewport columns initially but they will be updated once the actual width is measured. I think this scenario is unlikely so I removed clampColumnWidth. getColumnWidthForMeasurement handles it using css clamp function now. I am okay with adding it back if you prefer
| prevGridWidthRef.current = gridWidth; | ||
| updateMeasuredAndResizedWidths(); | ||
| }); | ||
| useLayoutEffect(updateMeasuredAndResizedWidths); |
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.
Cannot inline this function as exhaustive deps rule complains. I can add useMemo but not sure it is worth
| return widthWithUnit; | ||
| } | ||
|
|
||
| const hasMaxWidth = maxWidth != null; |
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.
Do we need to handle maxWidth here if we set it on measuring cells?
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.
Not 100% sure. My understanding is that grid computes the column widths and then apply min/max style from the measuring cell. This is the reason there was some left over space in the bug you found. I will check again
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.
Also, resizing does not work properly without this as clampColumnWidth helper was removed
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.
Found another edge case.
Co-authored-by: Nicolas Stepien <[email protected]>
#3746 (comment)
I have removed
clampColumnWidthhelper and now the browser handle theminWidth/maxWidthusing the following logicclamp/maxwhen possibleminmaxwhen possibleminWidth/maxWidthon the measuring cell