Skip to content

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

Draft
wants to merge 46 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
46 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
38e8d2f
Initial commit
amanmahajan7 Mar 21, 2025
6ced075
Revert example
amanmahajan7 Mar 21, 2025
db17e49
fix comment
amanmahajan7 Mar 21, 2025
f4e0cf2
Add return type
amanmahajan7 Mar 21, 2025
d4dfc55
comment
amanmahajan7 Mar 21, 2025
6ca4dd5
tweak
amanmahajan7 Mar 21, 2025
389c01e
Add a comment
amanmahajan7 Mar 21, 2025
2650ccb
use `state` instead of `ref`
amanmahajan7 Mar 21, 2025
2b338b0
use `clamp` and `max` css functions
amanmahajan7 Mar 21, 2025
20dd703
Remove width
amanmahajan7 Mar 21, 2025
106842e
is `flushSync` needed
amanmahajan7 Mar 21, 2025
52b9237
Add back max-width
amanmahajan7 Mar 21, 2025
a56574c
`fit-content` does not do what I think it does
amanmahajan7 Mar 21, 2025
c90685c
handle `minWidth`
amanmahajan7 Mar 21, 2025
4789821
Use `CSS.supports`
amanmahajan7 Mar 23, 2025
d9063f6
Fix tests
amanmahajan7 Mar 24, 2025
fe0d696
Fix node test
amanmahajan7 Mar 24, 2025
d3b412d
Tweak logic
amanmahajan7 Mar 24, 2025
e6bb382
Add `ResizedWidth` type
amanmahajan7 Mar 24, 2025
03d9f61
Add back `flushSync`
amanmahajan7 Mar 25, 2025
0bc54d9
Merge branch 'main' into am-double-double-click
amanmahajan7 Apr 1, 2025
2522c3d
Merge branch 'am-double-double-click' into am-refactor-minmax-logic
amanmahajan7 Apr 1, 2025
833b52d
Move function
amanmahajan7 Apr 1, 2025
b497648
-1 loop
amanmahajan7 Apr 3, 2025
42acc94
Tweak
amanmahajan7 Apr 3, 2025
58b38d3
Merge branch 'main' into am-double-double-click
amanmahajan7 Apr 7, 2025
45bbadd
Merge branch 'am-double-double-click' into am-refactor-minmax-logic
amanmahajan7 Apr 7, 2025
a905deb
Merge branch 'main' into am-refactor-minmax-logic
amanmahajan7 Apr 8, 2025
ea69fac
Revert changes
amanmahajan7 Apr 8, 2025
a4672d6
Fix tests
amanmahajan7 Apr 9, 2025
852a7b5
Fix tests
amanmahajan7 Apr 9, 2025
b79becf
Update test/browser/column/resizable.test.tsx
amanmahajan7 Apr 10, 2025
5bc91a8
Merge branch 'main' into am-refactor-minmax-logic
amanmahajan7 Apr 10, 2025
aabb956
Merge branch 'main' into am-refactor-minmax-logic
amanmahajan7 Apr 10, 2025
96887bb
Rename
amanmahajan7 Apr 10, 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
4 changes: 1 addition & 3 deletions src/HeaderCell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { css } from '@linaria/core';

import { useRovingTabIndex } from './hooks';
import {
clampColumnWidth,
getCellClassname,
getCellStyle,
getHeaderCellRowSpan,
Expand Down Expand Up @@ -121,8 +120,7 @@ export default function HeaderCell<R, SR>({
const offset = isRtl ? event.clientX - left : right - event.clientX;
function onPointerMove(event: PointerEvent) {
const { width, right, left } = headerCell.getBoundingClientRect();
let newWidth = isRtl ? right + offset - event.clientX : event.clientX + offset - left;
newWidth = clampColumnWidth(newWidth, column);
const newWidth = isRtl ? right + offset - event.clientX : event.clientX + offset - left;
if (width > 0 && newWidth !== width) {
onColumnResize(column, newWidth);
}
Expand Down
6 changes: 2 additions & 4 deletions src/hooks/useCalculatedColumns.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useMemo } from 'react';

import { clampColumnWidth, max, min } from '../utils';
import { max, min } from '../utils';
import type { CalculatedColumn, CalculatedColumnParent, ColumnOrColumnGroup, Omit } from '../types';
import { renderValue } from '../cellRenderers';
import { SELECT_COLUMN_KEY } from '../Columns';
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

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

} else {
if (typeof width === 'string') {
// This is a placeholder width so we can continue to use virtualization.
// The actual value is set after the column is rendered
width = column.minWidth;
Expand Down
63 changes: 52 additions & 11 deletions src/hooks/useColumnWidths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,28 +29,26 @@ export function useColumnWidths<R, SR>(
const newTemplateColumns = [...templateColumns];
const columnsToMeasure: string[] = [];

for (const { key, idx, width } of viewportColumns) {
for (const column of viewportColumns) {
const { key, idx, width } = column;
if (key === columnToAutoResize?.key) {
newTemplateColumns[idx] =
columnToAutoResize.width === 'max-content'
? columnToAutoResize.width
: `${columnToAutoResize.width}px`;
newTemplateColumns[idx] = getColumnWidthForMeasurement(columnToAutoResize.width, column);
columnsToMeasure.push(key);
} else if (
typeof width === 'string' &&
(ignorePreviouslyMeasuredColumns || !measuredColumnWidths.has(key)) &&
// If the column is resized by the user, we don't want to measure it again
!resizedColumnWidths.has(key)
) {
newTemplateColumns[idx] = width;
newTemplateColumns[idx] = getColumnWidthForMeasurement(width, column);
columnsToMeasure.push(key);
}
}

const gridTemplateColumns = newTemplateColumns.join(' ');

useLayoutEffect(updateMeasuredWidths);
useLayoutEffect(updateMeasuredAndResizedWidths);
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Apr 3, 2025

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


function updateMeasuredWidths() {
function updateMeasuredAndResizedWidths() {
setPreviousGridWidth(gridWidth);
if (columnsToMeasure.length === 0) return;

Expand Down Expand Up @@ -112,8 +110,7 @@ export function useColumnWidths<R, SR>(

if (onColumnResize) {
const previousWidth = resizedColumnWidths.get(resizingKey);
const newWidth =
typeof nextWidth === 'number' ? nextWidth : measureColumnWidth(gridRef, resizingKey);
const newWidth = measureColumnWidth(gridRef, resizingKey);
if (newWidth !== undefined && newWidth !== previousWidth) {
onColumnResize(column, newWidth);
}
Expand All @@ -131,3 +128,47 @@ function measureColumnWidth(gridRef: React.RefObject<HTMLDivElement | null>, key
const measuringCell = gridRef.current?.querySelector(selector);
return measuringCell?.getBoundingClientRect().width;
}

function getColumnWidthForMeasurement<R, SR>(
width: number | string,
{ minWidth, maxWidth }: CalculatedColumn<R, SR>
) {
const widthWithUnit = typeof width === 'number' ? `${width}px` : width;

// don't break in Node.js (SSR) and jsdom
if (typeof CSS === 'undefined') {
return widthWithUnit;
}

const hasMaxWidth = maxWidth != null;
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found another edge case.

const clampedWidth = hasMaxWidth
? `clamp(${minWidth}px, ${widthWithUnit}, ${maxWidth}px)`
: `max(${minWidth}px, ${widthWithUnit})`;

// clamp() and max() do not handle all the css grid column width values
if (isValidCSSGridColumnWidth(clampedWidth)) {
return clampedWidth;
}

if (
hasMaxWidth &&
// ignore maxWidth if it less than minWidth
maxWidth >= minWidth &&
// we do not want to use minmax with max-content as it
// can result in width being larger than max-content
widthWithUnit !== 'max-content'
) {
// We are setting maxWidth on the measuring cell but the browser only applies
// it after all the widths are calculated. This results in left over space in some cases.
const minMaxWidth = `minmax(${widthWithUnit}, ${maxWidth}px)`;
if (isValidCSSGridColumnWidth(minMaxWidth)) {
return minMaxWidth;
}
}

return isValidCSSGridColumnWidth(widthWithUnit) ? widthWithUnit : 'auto';
}

function isValidCSSGridColumnWidth(width: string) {
return CSS.supports('grid-template-columns', width);
}
16 changes: 1 addition & 15 deletions src/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { CalculatedColumn, CalculatedColumnOrColumnGroup, Maybe } from '../types';
import type { CalculatedColumnOrColumnGroup, Maybe } from '../types';

export * from './colSpanUtils';
export * from './domUtils';
Expand All @@ -18,20 +18,6 @@ export function assertIsValidKeyGetter<R, K extends React.Key>(
}
}

export function clampColumnWidth<R, SR>(
width: number,
{ minWidth, maxWidth }: CalculatedColumn<R, SR>
): number {
width = max(width, minWidth);

// ignore maxWidth if it less than minWidth
if (typeof maxWidth === 'number' && maxWidth >= minWidth) {
return min(width, maxWidth);
}

return width;
}

export function getHeaderCellRowSpan<R, SR>(
column: CalculatedColumnOrColumnGroup<R, SR>,
rowIdx: number
Expand Down
5 changes: 4 additions & 1 deletion test/browser/column/resizable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,15 @@ test('should resize column when dragging the handle', async () => {
});

test('should use the maxWidth if specified', async () => {
setup<Row, unknown>({ columns, rows: [] });
const onColumnResize = vi.fn();
setup<Row, unknown>({ columns, rows: [], onColumnResize });
const grid = getGrid();
expect(onColumnResize).not.toHaveBeenCalled();
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' });
expect(onColumnResize).toHaveBeenCalledWith(expect.objectContaining(columns[1]), 400);
});

test('should use the minWidth if specified', async () => {
Expand Down
1 change: 1 addition & 0 deletions website/routes/CommonFeatures.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function getColumns(
{
key: 'country',
name: 'Country',
maxWidth: 150,
renderEditCell: (p) => (
<select
autoFocus
Expand Down