Repository navigation
fix(resize): enforce configured column minWidth - #1378
Conversation
✅ Deploy Preview for react-data-table-component ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Closing this pull request for now. |
|
Hey @kitiho good catch on the bug. Definitely a bug. The fix doesn't hold up though when I verified this in the browser. I think this would be the simpler change, and the tests would need updating for
I see you commented that you were closing this but it's still open, so if you |
|
Updated — thanks for the review.
Verified with |
38b3f52 to
41b64c2
Compare
| defaultSortAsc, | ||
| ); | ||
|
|
||
| // ── Column resize state ──────────────────────────────────────────────────── |
| onColumnResizeRef.current = onColumnResize; | ||
| isRTLRef.current = isRTL; | ||
| }); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
useLayoutEffect gains nothing over useEffect here, but more importantly the effect is unnecessary
|
Thanks for your review ~ But actually I still think the best approach is to pass column.minWidth from TableCol when a resize gesture starts, and parse that value directly in useColumnResize. For example: // TableCol.tsx
onResizeStart(column.id!, e, column.minWidth);// useColumnResize.ts
const configuredMin =
configuredMinWidth != null ? Number.parseFloat(configuredMinWidth) : Number.NaN;
resizeRef.current = {
columnId,
startX: e.clientX,
startWidth,
minWidth: Math.max(40, Number.isFinite(configuredMin) ? configuredMin : 0),
};I think this is preferable because
|
|
Fair point on TableCol @kitiho, let's go with your version. Passing Ignore my two comments on the layout effect then, that code goes away with this. Tests still need work though. Sorry, I swapped the snippets in my earlier comment. The const mock = dataMock();
Object.defineProperty(headerCell, 'offsetWidth', { value: 300, configurable: true });
// 300 - 300 = 0, so the 40px floor applies.
fireEvent.pointerDown(handle, { clientX: 300 });
fireEvent.pointerMove(document, { clientX: 0 });
fireEvent.pointerUp(document);
expect(headerCell.style.maxWidth).toBe('40px');
// And the one that shouldn't clamp:
const mock = dataMock({ minWidth: '120px' });
Object.defineProperty(headerCell, 'offsetWidth', { value: 300, configurable: true });
// 300 - 50 = 250, above the minimum, left alone.
fireEvent.pointerDown(handle, { clientX: 300 });
fireEvent.pointerMove(document, { clientX: 250 });
fireEvent.pointerUp(document);
expect(headerCell.style.maxWidth).toBe('250px'); |
41b64c2 to
97b18f6
Compare
97b18f6 to
a318b9d
Compare
|
Thanks for the detailed review and for clarifying the test cases. @jbetancur I updated the implementation to pass I also updated the tests to cover:
All checks pass locally: Thanks again for the guidance — it made the implementation much simpler and more focused. |
|
Thank you @kitiho 8.8.2 should be available shortly! |

Bug fix
The resizable-column documentation states that
column.minWidthclamps how small a user can drag a column. However, the resize handler currently only applies its fixed 40 px floor and ignores the configuredminWidth. As a result, a column configured withminWidth: "120px"can still be dragged to 40 px.This PR makes the implementation match the documented behavior.
Changes
column.minWidthVerification
npm testnpm run typechecknpm run lintnpm run buildRelates to #1377