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.

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 column resize with pinned rows #10229

Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Sep 4, 2023

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse feature: Row pinning Related to the data grid Row pinning feature feature: Column resize labels Sep 4, 2023
@mui-bot
Copy link

mui-bot commented Sep 4, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10229--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -195.7 209.8 -65.2 -58.56 144.069
Sort 100k rows ms 757.7 1,320.4 1,266.7 1,157.24 203.125
Select 100k rows ms 620.2 850.1 729.5 737.78 80.076
Deselect 100k rows ms 141.7 279.6 194.9 193.2 48.871

Generated by 🚫 dangerJS against 71c212a

@cherniavskii cherniavskii marked this pull request as ready for review September 4, 2023 20:26
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

Works great 🎉

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!

@cherniavskii cherniavskii enabled auto-merge (squash) September 6, 2023 08:57
@cherniavskii cherniavskii merged commit 2be2bab into mui:master Sep 6, 2023
2 checks passed
@cherniavskii cherniavskii deleted the fix-column-resize-with-pinned-rows branch September 6, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Column resize feature: Row pinning Related to the data grid Row pinning feature regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Column resize doesn't work properly with pinned rows
4 participants