-
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 ReportAttention: 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
@@ -177,9 +177,7 @@ export function useCalculatedColumns<R, SR>({ | |||
for (const column of columns) { | |||
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
clampColumnWidth
helper and now the browser handle theminWidth/maxWidth
using the following logicclamp/max
when possibleminmax
when possibleminWidth/maxWidth
on the measuring cell