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.
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.
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.
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.
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.
Do we need to handle maxWidth here if we set it on measuring cells?
There was a problem hiding this comment.
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.
Also, resizing does not work properly without this as clampColumnWidth helper was removed
There was a problem hiding this comment.
Found another edge case.
Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
#3746 (comment)
I have removed
clampColumnWidthhelper and now the browser handle theminWidth/maxWidthusing the following logicclamp/maxwhen possibleminmaxwhen possibleminWidth/maxWidthon the measuring cell