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

[DataGrid] getCellElement does not work with pinnedColumns. #7093

Closed
2 tasks done
yaredtsy opened this issue Dec 5, 2022 · 22 comments · Fixed by #7354
Closed
2 tasks done

[DataGrid] getCellElement does not work with pinnedColumns. #7093

yaredtsy opened this issue Dec 5, 2022 · 22 comments · Fixed by #7354
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. plan: Premium Impact at least one Premium user

Comments

@yaredtsy
Copy link
Contributor

yaredtsy commented Dec 5, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/zen-benz-e7bky2?file=/demo.tsx

Steps:

  1. click on one of the cells that are not in the pinned column section.
  2. click on print active element button.
  3. check the log, its logging null

Current behavior 😯

getCellElement is returning null when I use it with pinnedColumns.

Expected behavior 🤔

it should return an HTML element.

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID 💳 (optional)

No response

@yaredtsy yaredtsy added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 5, 2022
@yaredtsy yaredtsy changed the title [DataGrid] getCellElement does not work with pinned column. [DataGrid] getCellElement does not work with pinnedColumns. Dec 5, 2022
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 5, 2022
@cherniavskii cherniavskii added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 5, 2022
@cherniavskii
Copy link
Member

Thanks @yaredtsy

Given that the row is rendered by multiple DOM elements when using pinned columns, we should use a combined query selector rather than two separate selectors when querying the DOM node:

diff --git a/packages/grid/x-data-grid/src/utils/domUtils.ts b/packages/grid/x-data-grid/src/utils/domUtils.ts
index d27463a..1357bff 100644
--- a/packages/grid/x-data-grid/src/utils/domUtils.ts
+++ b/packages/grid/x-data-grid/src/utils/domUtils.ts
@@ -35,18 +35,18 @@ export function getGridColumnHeaderElement(root: Element, field: string) {
   );
 }
 
+function getGridRowElementSelector(id: GridRowId): string {
+  return `.${gridClasses.row}[data-id="${escapeOperandAttributeSelector(String(id))}"]`;
+}
+
 export function getGridRowElement(root: Element, id: GridRowId) {
-  return root.querySelector<HTMLDivElement>(
-    `.${gridClasses.row}[data-id="${escapeOperandAttributeSelector(String(id))}"]`,
-  );
+  return root.querySelector<HTMLDivElement>(getGridRowElementSelector(id));
 }
 
 export function getGridCellElement(root: Element, { id, field }: { id: GridRowId; field: string }) {
-  const row = getGridRowElement(root, id);
-  if (!row) {
-    return null;
-  }
-  return row.querySelector<HTMLDivElement>(
-    `.${gridClasses.cell}[data-field="${escapeOperandAttributeSelector(field)}"]`,
-  );
+  const rowSelector = getGridRowElementSelector(id);
+  const cellSelector = `.${gridClasses.cell}[data-field="${escapeOperandAttributeSelector(
+    field,
+  )}"]`;
+  return root.querySelector<HTMLDivElement>(`${rowSelector} ${cellSelector}`);
 }

@m4theushw
Copy link
Member

Playing the devil's advocate here. What should apiRef.current.getGridCellElement return if inside the Email column there's another DataGrid containing a Name column. Not only using a single selector to check if the cell is a child of the row, but we need to also only return cells that belong to apiRef.current.rootElementRef.current. Solving this problem is a step towards #4243

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 6, 2022

do you mean something like this? https://codesandbox.io/s/zen-benz-e7bky2?file=/demo.tsx. apiRef.current.getGridCellElement is working fine.

@m4theushw
Copy link
Member

Check this CodeSandbox: https://codesandbox.io/s/wild-pine-49uown?file=/demo.tsx

The main grid doesn't have an Age column but apiRef.current.getGridCellElement(id, 'age') returns the cell from the child grid.

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 6, 2022

oh yeah, it's returning the first element when I change the order of age and email column it's returning the first element that comes first. could this be solved by generating a unique id every time a Datagrid is created. and store it in row and cell elements like data-parentId.

@cherniavskii
Copy link
Member

cherniavskii commented Dec 6, 2022

Right, we can exclude elements from nested grids with something like this:

const selector = `${rowSelector} ${cellSelector}`;
return root.querySelector(`${selector}:not(.${gridClasses.root} .${gridClasses.root} ${selector})`);

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 6, 2022

return root.querySelector(${selectorQuery}:not(${gridClasses.root} ${gridClasses.root} ${selectorQuery}));

this means that's not in nested DataGrid right? what if I am working in the nested DataGrid

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 6, 2022

I have tested giving Datagrid a unique id and it seems to be working
Demo: https://codesandbox.io/s/lucid-cerf-buxl0g?file=/demo.tsx

I have exposed gridId API you can change it, you can make it the same as the nested datagrid, and check its effect.
also, you must pass the Datagrid id manually to apiRef.current.getCellElement(cell?.id, "age", "gridId").
I have made like so it can be easier to test and check the effect.

@cherniavskii
Copy link
Member

this means that's not in nested DataGrid right?

Yeah, might be. I thought it would work because of root.querySelector (not document.querySelector), but it will probably consider the tree above the root as well.

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 8, 2022

using the unique id for Datagrid also solves the datagrid in datagrid problem #4243
Before: https://codesandbox.io/s/icy-flower-h047x1

After: https://codesandbox.io/s/festive-hypatia-y96003?file=/demo.tsx
Demo from 4243 https://codesandbox.io/s/debug-modal-with-datagrid-forked-2lb9ge?file=/src/TinyInfo.tsx.

should I create a PR for it?

@cherniavskii
Copy link
Member

cherniavskii commented Dec 8, 2022

I have exposed gridId API you can change it

We can use apiRef.current.instanceId instead to avoid passing gridId prop.

@m4theushw
Copy link
Member

It seems to me that passing data-gridId to the cell element is only to make writing the selector easier. It won't be used in other places. If an user has passed a custom cell component, and it doesn't add this attribute, then everything relaying on getCellElement won't work. Instead, we could solve this problem by looking if between the given root and the candidate cell there's no other grid.

export function getGridCellElement(root: Element, { id, field }: { id: GridRowId; field: string }) {
  const isChildOfRoot = (element: HTMLElement) => {
    let parent = element.parentElement;
    let found = false;
    while (!found && parent !== null) {
      if (parent?.classList.contains('MuiDataGrid-root')) {
        found = true;
      } else {
        parent = parent?.parentElement;
      }
    }
    return parent === root;
  };

  const candidates = root.querySelectorAll<HTMLDivElement>(
    `.${gridClasses.row}[data-id="${escapeOperandAttributeSelector(String(id))}"] > .${
      gridClasses.cell
    }[data-field="${escapeOperandAttributeSelector(field)}"]`,
  );
  const filteredCandidates = Array.from(candidates).filter(isChildOfRoot);
  return filteredCandidates.length ? filteredCandidates[0] : null;
}

Something similar could be done in findParentElementFromClassName and close #4243

@cherniavskii
Copy link
Member

Also, TreeWalker might be useful here

@m4theushw
Copy link
Member

@cherniavskii Cool, I didn't know about this API. The only problem is that it doesn't seem to have a easy way to know the distance from a node to the root, so we'll need something similar to isChildOfRoot.

@yaredtsy
Copy link
Contributor Author

If an user has passed a custom cell component, and it doesn't add this attribute, then everything relying on getCellElement won't work

what about passing the apiRef.current.instanceId to Row's. and the cell must be a direct descendent of that row

 const cell = (event.target as HTMLDivElement).closest(
        `[data-parentid="${apiRef.current.instanceId}"] > .${gridClasses.cell}`,
      );

or add the id to virtualScroller and the cell must be a direct child of the row which is a direct child of the virtualScoller with the grid instance id.

const cell = (event.target as HTMLDivElement).closest(
      `[data-parentid="${apiRef.current.instanceId}"] > .${gridClasses.row} > .${gridClasses.cell}`,
);

Something similar could be done in findParentElementFromClassName.

findParentElementFromClassName is using closest() . I do not know if it's possible to apply the same logic. if we did not modify the selector it always returns the cell element.

@m4theushw
Copy link
Member

I prefer to not add new attributes to DOM elements unless there's no other solution. data-parentid is being added only to make the selector in getGridCellElement easier to write.

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Dec 16, 2022

I found another way. if we calculate the exact depth of the Datagrid we can query for cells in that depth only. bt excluding the datagrids that are above and below

  let depth = 0;
  let current: Element = root;
  while (current?.parentElement != null) {
    if (current?.parentElement?.classList.contains(gridClasses.root)) {
      depth += 1;
    }
    current = current?.parentElement;
  }

we can query getGridCellElement only in grid that is depth number of gridClasses.root child and childs morethan depth + 1 .

  let parent = `.${gridClasses.root}`;

  for (let i = 0; i < depth; i += 1) {
    parent += ` .${gridClasses.root}`;
  }

  return root.querySelector<HTMLDivElement>(
    `${parent} ${selector}:not(${parent}  .${gridClasses.root} ${selector})`,
  );
}

and for findParentElementFromClassName query for ancestor elements that do not have depth + 1 of gridClasses.root parents

let parent = `.${gridClasses.root} .${gridClasses.root}`;
  for (let i = 0; i < depth; i += 1) {
    parent += ` .${gridClasses.root}`;
  }
  return elem.closest(`.${className}:not( ${parent} .${className})`);

I have tested it out everything works.

@yaredtsy
Copy link
Contributor Author

@yaredtsy
Copy link
Contributor Author

should I create a PR as a draft so you can guys can review the implementation?

@cherniavskii
Copy link
Member

@yaredtsy are there some specific use cases where you could reproduce this issue?
If not - it's not a high priority and we can address it later after releasing the v6 stable

@yaredtsy
Copy link
Contributor Author

hey @cherniavskii , no I do not have any use cases for it. it can wait

@cherniavskii
Copy link
Member

We just got the first report for this issue in #7819

@cherniavskii cherniavskii added the plan: Premium Impact at least one Premium user label Feb 6, 2023
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Feb 6, 2023
cherniavskii pushed a commit that referenced this issue Feb 6, 2023
cherniavskii pushed a commit to cherniavskii/mui-x that referenced this issue Feb 6, 2023
@cherniavskii cherniavskii moved this from 🆕 Needs refinement to 📋 Backlog in MUI X Data Grid Feb 6, 2023
@cherniavskii cherniavskii moved this from 📋 Backlog to ✅ Done in MUI X Data Grid Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. plan: Premium Impact at least one Premium user
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants