Skip to content

Fix column auto resize #3746

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.

Sign up for GitHub

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

Merged
amanmahajan7 merged 18 commits into from
Apr 8, 2025
Merged

Fix column auto resize #3746

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
005c9f2
Use state to handle column auto resize
amanmahajan7 Mar 19, 2025
b66f3bd
Optimize
amanmahajan7 Mar 19, 2025
51c2bdd
Cleanup
amanmahajan7 Mar 20, 2025
d4afa2b
Test
amanmahajan7 Mar 20, 2025
ccaa437
Fix tests
amanmahajan7 Mar 20, 2025
69c46a2
Add flex columns test, make tests more robust
amanmahajan7 Mar 20, 2025
f60e759
Improve coverage
amanmahajan7 Mar 20, 2025
f736a3b
test `onColumnResize`
amanmahajan7 Mar 20, 2025
0e90906
Tweak
amanmahajan7 Mar 20, 2025
523ec5a
Rename helper
amanmahajan7 Mar 20, 2025
8a6bb28
Tweak comment
amanmahajan7 Mar 20, 2025
0bc54d9
Merge branch 'main' into am-double-double-click
amanmahajan7 Apr 1, 2025
58b38d3
Merge branch 'main' into am-double-double-click
amanmahajan7 Apr 7, 2025
8bae511
Address comments
amanmahajan7 Apr 8, 2025
012a2b9
tweak test
nstepien Apr 8, 2025
3f04905
Revert "tweak test"
amanmahajan7 Apr 8, 2025
21205c8
Use `flushSync`
amanmahajan7 Apr 8, 2025
06d7e92
Address comments
amanmahajan7 Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 61 additions & 39 deletions src/hooks/useColumnWidths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useLayoutEffect, useRef } from 'react';
import { flushSync } from 'react-dom';
import { useLayoutEffect, useRef, useState } from 'react';

import type { CalculatedColumn, StateSetter } from '../types';
import type { DataGridProps } from '../DataGrid';
Expand All @@ -16,6 +15,7 @@ export function useColumnWidths<R, SR>(
setMeasuredColumnWidths: StateSetter<ReadonlyMap<string, number>>,
onColumnResize: DataGridProps<R, SR>['onColumnResize']
) {
const [columnToAutoResize, setColumnToAutoResize] = useState<string | null>(null);
const prevGridWidthRef = useRef(gridWidth);
Copy link
Copy Markdown
Contributor

@Wroud Wroud Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amanmahajan7
I think you better not use ref for the previous grid width. There is a bug with the initial render when:

  1. table rendered with a different width from the previous
  2. columns calculated + updated
  3. table rendered with a different width from the previous
  4. columns calculated but aren't updated (because of the same widths) -> there are no re-render -> cells will have "measurement" size (like "max-content") and will change size on content size

This issue is reflected in my video from this comment: #3723 (comment)

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this issues still exists in my PR? I think the issue you are describing was related to #3724 (comment)

const columnsCanFlex: boolean = columns.length === viewportColumns.length;
// Allow columns to flex again when...
Expand All @@ -26,7 +26,10 @@ export function useColumnWidths<R, SR>(
const columnsToMeasure: string[] = [];

for (const { key, idx, width } of viewportColumns) {
if (
if (key === columnToAutoResize) {
newTemplateColumns[idx] = 'max-content';
columnsToMeasure.push(key);
} else if (
typeof width === 'string' &&
(ignorePreviouslyMeasuredColumns || !measuredColumnWidths.has(key)) &&
!resizedColumnWidths.has(key)
Expand All @@ -40,62 +43,81 @@ export function useColumnWidths<R, SR>(

useLayoutEffect(() => {
prevGridWidthRef.current = gridWidth;
updateMeasuredWidths(columnsToMeasure);
updateMeasuredAndResizedWidths();
});

function updateMeasuredWidths(columnsToMeasure: readonly string[]) {
if (columnsToMeasure.length === 0) return;
Comment thread
nstepien marked this conversation as resolved.
function updateMeasuredAndResizedWidths() {
Comment thread
nstepien marked this conversation as resolved.
Outdated
if (columnsToMeasure.length > 0) {
setMeasuredColumnWidths((measuredColumnWidths) => {
const newMeasuredColumnWidths = new Map(measuredColumnWidths);
let hasChanges = false;

for (const key of columnsToMeasure) {
const measuredWidth = measureColumnWidth(gridRef, key);
hasChanges ||= measuredWidth !== measuredColumnWidths.get(key);
if (measuredWidth === undefined) {
newMeasuredColumnWidths.delete(key);
} else {
newMeasuredColumnWidths.set(key, measuredWidth);
}
}

setMeasuredColumnWidths((measuredColumnWidths) => {
const newMeasuredColumnWidths = new Map(measuredColumnWidths);
let hasChanges = false;
return hasChanges ? newMeasuredColumnWidths : measuredColumnWidths;
});
}

for (const key of columnsToMeasure) {
const measuredWidth = measureColumnWidth(gridRef, key);
hasChanges ||= measuredWidth !== measuredColumnWidths.get(key);
if (measuredWidth === undefined) {
newMeasuredColumnWidths.delete(key);
} else {
newMeasuredColumnWidths.set(key, measuredWidth);
if (columnToAutoResize !== null) {
setColumnToAutoResize(null);
setResizedColumnWidths((resizedColumnWidths) => {
const oldWidth = resizedColumnWidths.get(columnToAutoResize);
const newWidth = measureColumnWidth(gridRef, columnToAutoResize);
if (newWidth !== undefined && oldWidth !== newWidth) {
const newResizedColumnWidths = new Map(resizedColumnWidths);
newResizedColumnWidths.set(columnToAutoResize, newWidth);
onColumnResize?.(viewportColumns.find((c) => c.key === columnToAutoResize)!, newWidth);
Comment thread
amanmahajan7 marked this conversation as resolved.
Outdated
return newResizedColumnWidths;
}
}

return hasChanges ? newMeasuredColumnWidths : measuredColumnWidths;
});
return resizedColumnWidths;
});
}
}

function handleColumnResize(column: CalculatedColumn<R, SR>, nextWidth: number | 'max-content') {
const { key: resizingKey } = column;
const newTemplateColumns = [...templateColumns];
const columnsToMeasure: string[] = [];

for (const { key, idx, width } of viewportColumns) {
if (resizingKey === key) {
const width = typeof nextWidth === 'number' ? `${nextWidth}px` : nextWidth;
newTemplateColumns[idx] = width;
} else if (columnsCanFlex && typeof width === 'string' && !resizedColumnWidths.has(key)) {
newTemplateColumns[idx] = width;
// remeasure all columns that can flex and are not resized by the user
for (const { key, width } of viewportColumns) {
if (
columnsCanFlex &&
resizingKey !== key &&
typeof width === 'string' &&
!resizedColumnWidths.has(key)
) {
columnsToMeasure.push(key);
}
}

gridRef.current!.style.gridTemplateColumns = newTemplateColumns.join(' ');
const measuredWidth =
typeof nextWidth === 'number' ? nextWidth : measureColumnWidth(gridRef, resizingKey)!;
if (columnsToMeasure.length > 0) {
Comment thread
nstepien marked this conversation as resolved.
Outdated
setMeasuredColumnWidths((measuredColumnWidths) => {
const newMeasuredColumnWidths = new Map(measuredColumnWidths);
for (const columnKey of columnsToMeasure) {
newMeasuredColumnWidths.delete(columnKey);
}
return newMeasuredColumnWidths;
});
}

// TODO: remove
// need flushSync to keep frozen column offsets in sync
// we may be able to use `startTransition` or even `requestIdleCallback` instead
flushSync(() => {
if (typeof nextWidth === 'number') {
Copy link
Copy Markdown
Collaborator

@nstepien nstepien Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this will correctly handle min/max-widths
We need to set the width, render, measure the cell, store the measured width instead of trusting nextWidth

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it does. I will double check
https://github.com/adazzle/react-data-grid/blob/main/src/HeaderCell.tsx#L128

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. We should put the min/max logic in one place

Copy link
Copy Markdown
Contributor

@Wroud Wroud Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min / max can't overflow in case styles are applied to the measurement cells correctly, and grid re-render is synchronized

I also found it helpful to use styles instead of JS for this, especially when you can have different styles for cells while measuring (when we set its size to "max-content" for example, and will measure its size after):

You want to have a grid with auto-sized columns but with some rules for this auto-size mechanism:
min: 50px
max: 300px

But at the same time, you want to be able to resize columns to exceed these values manually.

In this situation, you will have different rules for measured cells and while measuring:
When measuring cells (like "max-content" or "auto"), we want to apply these rules to that cell
but when we resize manually, we don't

.measuringCellClassname {
  min-width: 20px;
}
.measuringCellClassname[measuring] {
  min-width: 50px;
  max-width: 300px
}

This is why I've added this code also:
https://github.com/adazzle/react-data-grid/pull/3724/files#diff-62e848727f791df23e85ccc2140643630d80c20c5140f073aba8f581eb122019L13-R26

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But at the same time, you want to be able to resize columns to exceed these values manually.

We want the same rules for bot cases. If maxWidth is specified then users should not be able to increase the width past that value.

I also found it helpful to use styles instead of JS
We already set min/maxWidth on the measuring cell
https://github.com/adazzle/react-data-grid/blob/main/src/utils/renderMeasuringCells.tsx#L18

Copy link
Copy Markdown
Contributor

@Wroud Wroud Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but I'm talking about cases when you have not specified maxWidth for the column, and this max-width only applies to the moment when we calculate the size of the column in cases when both are specified:
column.maxWidth: 100px
measuringCellClassname[measuring] { max-width: 200px; }
then it should not exceed 100px
But in case:
column.maxWidth: 100px
measuringCellClassname[measuring] { max-width: 50px; }
the auto-sized width will not exceed 50px but can be re-sized up to 100px manually

setResizedColumnWidths((resizedColumnWidths) => {
const newResizedColumnWidths = new Map(resizedColumnWidths);
newResizedColumnWidths.set(resizingKey, measuredWidth);
newResizedColumnWidths.set(resizingKey, nextWidth);
return newResizedColumnWidths;
});
updateMeasuredWidths(columnsToMeasure);
});

onColumnResize?.(column, measuredWidth);
onColumnResize?.(column, nextWidth);
Copy link
Copy Markdown
Contributor

@Wroud Wroud Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will suggest moving the onColumnResize call to the setter function to have the same possible behavior for resizing
https://github.com/adazzle/react-data-grid/pull/3746/files#diff-e002f181d41ce4fd6e070724347a44e4220d0332c37e5964d04cb3fc5d60d416R77
(or vice-versa)

} else {
setColumnToAutoResize(resizingKey);
}
}

return {
Expand Down
67 changes: 60 additions & 7 deletions test/browser/column/resizable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,48 +66,52 @@ test('cannot not resize or auto resize column when resizable is not specified',
test('should resize column when dragging the handle', async () => {
const onColumnResize = vi.fn();
setup<Row, unknown>({ columns, rows: [], onColumnResize });
const [, col2] = getHeaderCells();
const grid = getGrid();
expect(onColumnResize).not.toHaveBeenCalled();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px' });
const [, col2] = getHeaderCells();
Copy link
Copy Markdown
Collaborator

@nstepien nstepien Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, surely the cell element should still be in the document if we get it earlier, no?

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be. I will investigate in a separate PR

await resize({ column: col2, resizeBy: -50 });
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 150px' });
expect(onColumnResize).toHaveBeenCalledExactlyOnceWith(expect.objectContaining(columns[1]), 150);
});

test('should use the maxWidth if specified', async () => {
setup<Row, unknown>({ columns, rows: [] });
const [, col2] = getHeaderCells();
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px ' });
const [, col2] = getHeaderCells();
await resize({ column: col2, resizeBy: 1000 });
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 400px' });
});

test('should use the minWidth if specified', async () => {
setup<Row, unknown>({ columns, rows: [] });
const [, col2] = getHeaderCells();
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px' });
const [, col2] = getHeaderCells();
await resize({ column: col2, resizeBy: -150 });
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 100px' });
});

test('should auto resize column when resize handle is double clicked', async () => {
const onColumnResize = vi.fn();
setup<Row, unknown>({
columns,
rows: [
{
col1: 1,
col2: 'a'.repeat(50)
}
]
],
onColumnResize
});
const [, col2] = getHeaderCells();
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px' });
const [, col2] = getHeaderCells();
await autoResize(col2);
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 327.703px' });
// This is called twice in strict mode
Copy link
Copy Markdown
Collaborator

@nstepien nstepien Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it? Do we not check to only call the function if the width has changed?

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 amanmahajan7 Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does call it twice. May be the state updates after the useLayoutEffect has run twice
https://github.com/adazzle/react-data-grid/blob/8a6bb2873ccf76cd340cd6139b0278d4426c1924/src/hooks/useColumnWidths.ts#L69

expect(onColumnResize).toHaveBeenCalledWith(expect.objectContaining(columns[1]), 327.703125);
});

test('should use the maxWidth if specified on auto resize', async () => {
Expand All @@ -120,9 +124,9 @@ test('should use the maxWidth if specified on auto resize', async () => {
}
]
});
const [, col2] = getHeaderCells();
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px' });
const [, col2] = getHeaderCells();
await autoResize(col2);
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 400px' });
});
Expand All @@ -137,9 +141,58 @@ test('should use the minWidth if specified on auto resize', async () => {
}
]
});
const [, col2] = getHeaderCells();
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 200px' });
const [, col2] = getHeaderCells();
await autoResize(col2);
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '100px 100px' });
});

test('should remeasure flex columns when resizing a column', async () => {
const onColumnResize = vi.fn();
setup<
{
readonly col1: string;
readonly col2: string;
readonly col3: string;
},
unknown
>({
columns: [
{
key: 'col1',
name: 'col1',
resizable: true
},
{
key: 'col2',
name: 'col2',
resizable: true
},
{
key: 'col3',
name: 'col3',
resizable: true
}
],
rows: [
{
col1: 'a'.repeat(10),
col2: 'a'.repeat(10),
col3: 'a'.repeat(10)
}
],
onColumnResize
});
const grid = getGrid();
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '639.328px 639.328px 639.344px' });
const [col1] = getHeaderCells();
await autoResize(col1);
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '79.1406px 919.422px 919.438px' });
expect(onColumnResize).toHaveBeenCalled();
Comment thread
amanmahajan7 marked this conversation as resolved.
Outdated
onColumnResize.mockClear();
// onColumnResize is not called if width is not changed
await autoResize(col1);
await expect.element(grid).toHaveStyle({ gridTemplateColumns: '79.1406px 919.422px 919.438px' });
expect(onColumnResize).not.toHaveBeenCalled();
Comment thread
amanmahajan7 marked this conversation as resolved.
Outdated
});