Skip to content
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

[DataGridPro] Fix onColumnWidthChange being called twice on autosize #12140

Merged
merged 17 commits into from
Feb 27, 2024
Merged

[DataGridPro] Fix onColumnWidthChange being called twice on autosize #12140

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
70d8729
fix onColumnWidthChange called before autosize affects column width
shaharyar-shamshi Feb 19, 2024
6d09b48
fix onColumnWidthChange called before autosize affects column width
shaharyar-shamshi Feb 19, 2024
77c6ee8
native event stored instead of synthtic event
shaharyar-shamshi Feb 19, 2024
5c35631
native event stored instead of synthtic event
shaharyar-shamshi Feb 19, 2024
f4c7693
native event stored instead of synthtic event
shaharyar-shamshi Feb 20, 2024
8b96f02
native event stored instead of synthtic event
shaharyar-shamshi Feb 20, 2024
e38b370
native event stored instead of synthtic event
shaharyar-shamshi Feb 20, 2024
417c43d
native event stored instead of synthtic event
shaharyar-shamshi Feb 20, 2024
cd8cc5b
width of the new column added
shaharyar-shamshi Feb 20, 2024
e3b0d63
test case added
shaharyar-shamshi Feb 21, 2024
27f1ad5
test case added
shaharyar-shamshi Feb 22, 2024
69e1a42
test case added
shaharyar-shamshi Feb 22, 2024
ce71551
unnecessary comment removed
shaharyar-shamshi Feb 26, 2024
d56332d
typo fixed
shaharyar-shamshi Feb 27, 2024
57a4903
resize event fired for multiple column
shaharyar-shamshi Feb 27, 2024
ca1e9d5
column width trigger for only changed column
shaharyar-shamshi Feb 27, 2024
e80c76a
column width trigger for only changed column
shaharyar-shamshi Feb 27, 2024
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
30 changes: 30 additions & 0 deletions packages/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ export const useGridColumnResize = (
const logger = useGridLogger(apiRef, 'useGridColumnResize');

const colDefRef = React.useRef<GridStateColDef>();
const previousMouseClickEvent = React.useRef<MouseEvent>();
const columnHeaderElementRef = React.useRef<HTMLDivElement>();
const headerFilterElementRef = React.useRef<HTMLDivElement>();
const groupHeaderElementsRef = React.useRef<Element[]>([]);
Expand Down Expand Up @@ -378,6 +379,24 @@ export const useGridColumnResize = (
// eslint-disable-next-line @typescript-eslint/no-use-before-define
stopListening();

// Prevent double-clicks from being interpreted as two separate clicks
if (previousMouseClickEvent.current) {
const prevEvent = previousMouseClickEvent.current;
const prevTimeStamp = prevEvent.timeStamp;
const prevClientX = prevEvent.clientX;
const prevClientY = prevEvent.clientY;

// Check if the current event is part of a double-click
if (
nativeEvent.timeStamp - prevTimeStamp < 300 &&
nativeEvent.clientX === prevClientX &&
nativeEvent.clientY === prevClientY
) {
previousMouseClickEvent.current = undefined;
return;
}
}

if (colDefRef.current) {
apiRef.current.setColumnWidth(colDefRef.current.field, colDefRef.current.width!);
logger.debug(
Expand Down Expand Up @@ -610,6 +629,8 @@ export const useGridColumnResize = (
const doc = ownerDocument(apiRef.current.rootElementRef!.current);
doc.body.style.cursor = 'col-resize';

previousMouseClickEvent.current = event.nativeEvent;

doc.addEventListener('mousemove', handleResizeMouseMove);
doc.addEventListener('mouseup', handleResizeMouseUp);

Expand Down Expand Up @@ -702,6 +723,15 @@ export const useGridColumnResize = (
}

apiRef.current.updateColumns(newColumns);

newColumns.forEach((newColumn) => {
const width: number = newColumn.width as number;
apiRef.current.publishEvent('columnWidthChange', {
element: apiRef.current.getColumnHeaderElement(newColumn.field),
colDef: newColumn,
width,
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This will trigger for all columns regardless of if they have changed. You have the original columns in columns.

You can avoid typings like this, the inference knows it's a number:

const width: number = newColumn.width as number;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are doing something wrong this implementation is correct.

let me Explain in detail

There are two possibility of calling autosizeColumns function

  1. through doubleClick event Listener https://github.com/mui/mui-x/blob/next/packages/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx#L743 from here handleColumnSeparatorDoubleClick calls the autosizeColumns and over here it pass the exact column which is being resized
  2. At the time of mounting when user pass autoResizeOnMount prop https://github.com/mui/mui-x/blob/next/packages/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx#L716

here each of the column is being resized on mounting

In our case to solve this bug we only need to concentrate on case 1 when the autosizeColumns is called on double click.

So on double click event from https://github.com/mui/mui-x/blob/next/packages/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx#L661

we get only one column that needs to be resized and we need not check on all the columns.

Now in case 2 when autOResizeOnMount is called in that case all the columns should be resized and we need to decide whether to publish event or not.

So from both the case we can figure it out that either all of the column will be resized or only 1 will be resized.

if we want to disable the event publish on all column resize (case 2) we just need to add one condition

if (userOptions?.columns) {
          newColumns.forEach((newColumn) => {
            const width = newColumn.width;
            apiRef.current.publishEvent('columnWidthChange', {
              element: apiRef.current.getColumnHeaderElement(newColumn.field),
              colDef: newColumn,
              width,
            });
          });
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for an if guard, just check the array and dispatch one event for each column that actually changed. Compare newColumn.width !== columns[index].width in the .forEach loop to figure out which ones changed.

The autosize function is part of the public API and can be called programatically with any number of columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okk then we need to consider this I thought it is not the public api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

} finally {
apiRef.current.unstable_setColumnVirtualization(true);
isAutosizingRef.current = false;
Expand Down
23 changes: 23 additions & 0 deletions packages/x-data-grid-pro/src/tests/columns.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,29 @@ describe('<DataGridPro /> - Columns', () => {
expect(onColumnWidthChange.args[0][0].width).to.equal(120);
});

it('should call onColumnWidthChange with correct width after resizing and then clicking the separator', async () => {
const onColumnWidthChange = spy();
render(<Test onColumnWidthChange={onColumnWidthChange} columns={columns} />);
const separator = document.querySelector(`.${gridClasses['columnSeparator--resizable']}`)!;
fireEvent.mouseDown(separator, { clientX: 100 });
fireEvent.mouseMove(separator, { clientX: 110, buttons: 1 });
fireEvent.mouseMove(separator, { clientX: 120, buttons: 1 });
expect(onColumnWidthChange.callCount).to.equal(0);
fireEvent.mouseUp(separator);
clock.tick(0);
expect(onColumnWidthChange.callCount).to.equal(1);
expect(onColumnWidthChange.args[0][0].width).to.equal(120);
fireEvent.doubleClick(separator);
await microtasks();
expect(onColumnWidthChange.callCount).to.be.at.least(2);
const widthArgs = onColumnWidthChange.args.map((arg) => arg[0].width);
const isWidth116Present = widthArgs.some((width) => width === 116);
expect(isWidth116Present).to.equal(true);
const colDefWidthArgs = onColumnWidthChange.args.map((arg) => arg[0].colDef.width);
const isColDefWidth116Present = colDefWidthArgs.some((width) => width === 116);
expect(isColDefWidth116Present).to.equal(true);
});

it('should not affect other cell elements that are not part of the main DataGrid instance', () => {
render(
<Test
Expand Down
Loading