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

Jump to bottom

[DataGridPro] Fix column resize with pinned rows #10229

Merged
merged 4 commits into from
Sep 6, 2023
Merged

[DataGridPro] Fix column resize with pinned rows #10229

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20fbda7
fix column resize to work with pinned rows
cherniavskii Sep 1, 2023
2fee1a4
add unit test
cherniavskii Sep 4, 2023
a07701c
Merge branch 'master' into fix-column-resize-with-pinned-rows
cherniavskii Sep 6, 2023
71c212a
use CSS selector instead of childNodes
cherniavskii Sep 6, 2023
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
36 changes: 36 additions & 0 deletions packages/grid/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 @@ -177,6 +177,42 @@ describe('<DataGridPro /> - Columns', () => {
});
});

it('should work with pinned rows', () => {
render(
<Test
{...baselineProps}
pinnedRows={{
top: [{ id: 'top-0', brand: 'Reebok' }],
bottom: [{ id: 'bottom-0', brand: 'Asics' }],
}}
/>,
);
const separator = document.querySelector(`.${gridClasses['columnSeparator--resizable']}`)!;
const nonPinnedCell = getCell(1, 0);
const columnHeaderCell = getColumnHeaderCell(0);
const topPinnedRowCell = document.querySelector(
`.${gridClasses['pinnedRows--top']} [role="cell"][data-colindex="0"]`,
);
const bottomPinnedRowCell = document.querySelector(
`.${gridClasses['pinnedRows--bottom']} [role="cell"][data-colindex="0"]`,
);

fireEvent.mouseDown(separator, { clientX: 100 });
fireEvent.mouseMove(separator, { clientX: 150, buttons: 1 });

expect(columnHeaderCell).toHaveInlineStyle({ width: '150px' });
expect(nonPinnedCell).toHaveInlineStyle({ width: '150px' });
expect(topPinnedRowCell?.getBoundingClientRect().width).to.equal(150);
expect(bottomPinnedRowCell?.getBoundingClientRect().width).to.equal(150);

fireEvent.mouseUp(separator);

expect(columnHeaderCell).toHaveInlineStyle({ width: '150px' });
expect(nonPinnedCell).toHaveInlineStyle({ width: '150px' });
expect(topPinnedRowCell?.getBoundingClientRect().width).to.equal(150);
expect(bottomPinnedRowCell?.getBoundingClientRect().width).to.equal(150);
});

describe('flex resizing', () => {
before(function beforeHook() {
if (isJSDOM) {
Expand Down
47 changes: 22 additions & 25 deletions packages/grid/x-data-grid-pro/src/utils/domUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,28 @@ export function findGridCellElementsFromCol(col: HTMLElement, api: GridPrivateAp
const colIndex = Number(ariaColIndex) - 1;
const cells: Element[] = [];

const virtualScrollerContent = api.virtualScrollerRef?.current?.firstElementChild;
if (!virtualScrollerContent) {
return [];
}

const renderedRowElements = virtualScrollerContent.querySelectorAll(
`:scope > div > .${gridClasses.row}`, // Use > to ignore rows from detail panels
);

renderedRowElements.forEach((rowElement) => {
const rowId = rowElement.getAttribute('data-id');
if (!rowId) {
return;
}

let columnIndex = colIndex;

const cellColSpanInfo = api.unstable_getCellColSpanInfo(rowId, colIndex);
if (cellColSpanInfo && cellColSpanInfo.spannedByColSpan) {
columnIndex = cellColSpanInfo.leftVisibleCellIndex;
}
const cell = rowElement.querySelector(`[data-colindex="${columnIndex}"]`);
if (cell) {
cells.push(cell);
}
api.virtualScrollerRef?.current?.childNodes.forEach((child) => {
Copy link
Member

Choose a reason for hiding this comment

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

A small opportunity: We could possibly avoid one loop by targeting rows directly from the virtualScroller.

diff --git a/packages/grid/x-data-grid-pro/src/utils/domUtils.ts b/packages/grid/x-data-grid-pro/src/utils/domUtils.ts
index 42046bf72..b21304b32 100644
--- a/packages/grid/x-data-grid-pro/src/utils/domUtils.ts
+++ b/packages/grid/x-data-grid-pro/src/utils/domUtils.ts
@@ -28,9 +28,9 @@ export function findGridCellElementsFromCol(col: HTMLElement, api: GridPrivateAp
   const colIndex = Number(ariaColIndex) - 1;
   const cells: Element[] = [];
 
-  api.virtualScrollerRef?.current?.childNodes.forEach((child) => {
-    const renderedRowElements = (child as HTMLElement).querySelectorAll(
-      `:scope > div > .${gridClasses.row}`, // Use > to ignore rows from detail panels
+  if (api.virtualScrollerRef?.current) {
+    const renderedRowElements = api.virtualScrollerRef.current.querySelectorAll(
+      `div > .${gridClasses.row}`, // Use > to ignore rows from detail panels
     );
 
     renderedRowElements.forEach((rowElement) => {
@@ -50,7 +50,7 @@ export function findGridCellElementsFromCol(col: HTMLElement, api: GridPrivateAp
         cells.push(cell);
       }
     });
-  });
+  }
 
   return cells;
 }

Copy link
Member Author

Choose a reason for hiding this comment

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

This will break #9670 (the very same PR that caused this regression 😅)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, correct 🤦 I missed backtracking the original problem.

Just out of curiosity I tried updating the selector from div > .${gridClasses.row} to :scope > div > div > .${gridClasses.row} and it seems to work without breaking #9670.
Its a CSS-only solution, still I am not sure if it's the most elegant of the solutions, since the selector has become a bit longer than usual and also depends on a strict DOM hierarchy. 😆
Feel free to go with whatever you feel better.

Copy link
Member

Choose a reason for hiding this comment

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

Also, comment // Use > to ignore rows from detail panels could probably become // Use > to ignore rows from nested data grids as the grid could also be nested in a cell other than a detail panel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, that works, thanks!

const renderedRowElements = (child as HTMLElement).querySelectorAll(
`:scope > div > .${gridClasses.row}`, // Use > to ignore rows from detail panels
);

renderedRowElements.forEach((rowElement) => {
const rowId = rowElement.getAttribute('data-id');
if (!rowId) {
return;
}

let columnIndex = colIndex;

const cellColSpanInfo = api.unstable_getCellColSpanInfo(rowId, colIndex);
if (cellColSpanInfo && cellColSpanInfo.spannedByColSpan) {
columnIndex = cellColSpanInfo.leftVisibleCellIndex;
}
const cell = rowElement.querySelector(`[data-colindex="${columnIndex}"]`);
if (cell) {
cells.push(cell);
}
});
});

return cells;
Expand Down
Loading