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] Add possibility to pass in component for row and cell #2138

Closed
1 task done
Primajin opened this issue Jul 12, 2021 · 16 comments · Fixed by #2753
Closed
1 task done

[DataGrid] Add possibility to pass in component for row and cell #2138

Primajin opened this issue Jul 12, 2021 · 16 comments · Fixed by #2753
Labels
component: data grid This is the name of the generic UI component, not the React module!

Comments

@Primajin
Copy link

Primajin commented Jul 12, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

With many material UI components I can overwrite what element should be rendered in the end.
Passing in components allows me to pass in a special checkbox component via https://material-ui.com/api/data-grid/data-grid/#slots but no component for the rows and cells themselves.

Examples 🌈

<DataGrid
  components={{ RowWrapper: '<a/>', CellWrapper: '<span/>' }}
/>

should give me something like

image

Motivation 🔦

I would like to have rows / cells rendered as something else than a div and thus provide an element via components
For example to attach to the middle mouse click the row should be an anchor that I can point to the detail page of the data. A mere onClick handler doesn't fire the event.

Order id 💳

ORDER:25229

@Primajin Primajin added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 12, 2021
@dtassone
Copy link
Member

For cells, you can use the renderCell function on the column object. https://material-ui.com/components/data-grid/columns/#render-cell

@Primajin
Copy link
Author

@dtassone Hmm but that only renders stuff inside the cell - it does not change the cell element.

In the above example a <strong> tag is provided - if you check the code it will render the same old div with a strong inside. What I want to achieve is that in this example the div becomes a strong itself and gets all the classes applied.

image

So basically like you do with Typography - you can provide a p but make it look like a h1: https://material-ui.com/components/typography/#changing-the-semantic-element

component elementType   The component used for the root node. Either a string to use a HTML element or a component. Overrides the behavior of the variantMapping prop.

This would really help us in our app! 👍🏻

@dtassone dtassone added new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 13, 2021
@m4theushw
Copy link
Member

We can add a new Cell and Row slots to allow to pass custom components. The same for the componentsProps.

To listen for click events made by the middle button, there's a workaround involving attaching an auxclick event listener to the grid's root. To have it only firing when clicking the row, you just have to check if the event.target comes from an element inside a row. Here's a very basic example to give an idea how to do it:

export default function Basic() {
  const [gridRoot, setGridRoot] = React.useState(null);

  React.useEffect(() => {
    const handleAuxClick = (event) => {
      if (event.button === 1) {
        console.log("middle click");
      }
    };

    if (gridRoot) {
      gridRoot.addEventListener("auxclick", handleAuxClick);
    }

    return () => {
      if (gridRoot) {
        gridRoot.removeEventListener("auxclick", handleAuxClick);
      }
    };
  }, [gridRoot]);

  const handleRef = (el) => {
    setGridRoot(el);
  };

  return (
    <div style={{ height: 250, width: "100%" }}>
      <DataGrid
        ref={handleRef}
        columns={[{ field: "name" }]}
        rows={[
          { id: 1, name: "React" },
          { id: 2, name: "Material-UI" }
        ]}
        onAuxClick={(event) => console.log(event)}
      />
    </div>
  );
}

@Primajin
Copy link
Author

Primajin commented Jul 14, 2021

Thank you @m4theushw for the suggestion I was scratching my head for a while to figure out why your example doesn't work in our case - one tiny detail is missing: the scrollbar

Once it is present the middle mouse button event doesn't seem to be fired anymore in the table body 😢

Here is a test codesandbox: https://f4d2y.csb.app/
Source: https://codesandbox.io/s/middle-mouse-click-f4d2y?file=/demo.tsx

Do you have an idea?

EDIT: I guess I need to go with the regular old mousedown then?

@Primajin
Copy link
Author

Arrgh - using the middle click to trigger a window.open() will not open it in the background as it would when the event is just fired on a link - so then back to square one it would be great if we can overwrite the row to become a link so that the event is formed correctly 😕

@m4theushw
Copy link
Member

You have to call event.preventDefault() in the pointerdown event too: WICG/auxclick#17 (comment) It stops the autoscroll behavior when there's a scrollbar.

@Primajin
Copy link
Author

OK let me describe the bigger picture that I am actually trying to achieve:
The issue that I have is that I would like to link my rows to a detail page of that data.
When you normally click on it it will open the data in the current view - but I want to also allow to open it in a new tab/window.
When a user ctrl/meta clicks that row and I call window.open() it opens correctly in a new tab in the background
But when a user middle mouse clicks it the same line of window.open all of the sudden opens it in the foreground and no matter if I run any window.focus / blur whatever the browser ignores it and just stays focused on the new tab.
This is an unwanted behaviour. But maybe I'm doing something completely wrong here?

Now I can add an anchor element to each and every cell to get the behaviour that I want, since middle clicking an anchor behaves correctly - but this will unnecessarily pollute the DOM with hundreds of links when instead I could (via this issue) just turn the row into an anchor without adding any new dom nodes.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 17, 2021

I would like to have rows / cells rendered as something else than a div and thus provide an element via components

@Primajin Ok thanks for the extra detail of the use case. So you are actually not looking for being able to do ⬆️. If you do, you would have <a role="row"> which is invalid.

Capture d’écran 2021-07-17 à 17 33 48

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a. Based on https://stackoverflow.com/questions/17147821/how-to-make-a-whole-row-in-a-table-clickable-as-a-link, you might not be able to implement it the way to have tried.


Stripe's dashboard implements what you are looking for by rendering this DOM structure: row > cell > a. Why not do this?


The thread makes me think a bit of #1912 (comment)

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement and removed new feature New feature or request support: question Community support but can be turned into an improvement labels Jul 17, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid / X-Grid] Add possibility to pass in component for row and cell [DataGrid] Add possibility to pass in component for row and cell Jul 17, 2021
@Primajin
Copy link
Author

Hmm for row > cell > a it means since we have an XGrid with potentially many many rows and columns we need to bloat the DOM with so many links for each and every cell. Plus add extra styles since there can be margins and paddings between the cells that become "dead" areas that can't be clicked. (thats what we went with initially and wanted to find a way to optimize it)

Would a <div role="row"><a href={link}>{children}</a></div> work as a passed in component?

And pardon the heretic question - since it's a div grid and not a real <table> anyways - would it be so bad to allow a dev using the library to break the aria pattern?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2021

@Primajin Columns and rows are virtualized. Does x100 more <a> for the cells in the viewport really has a significant impact on performance? I don't think so.

Would a <div role="row"><a href={link}>{children}</a></div> work as a passed in component?

There are cells where you wouldn't want as a link wrapper: a > button is KO too. For instance, the column for checkbox selection, the column for actions. But yeah, if we expose enough parameters so you can conditionally render, it could work.
We would have to not move forward with point 1. of #1912 (comment).

It could work equally if you have a wrapper on top of your renderCell?

@Primajin
Copy link
Author

Yeah I guess that could work - I think I will need to play a bit more with this during my next workdays 👍🏻

@Primajin
Copy link
Author

Primajin commented Jul 20, 2021

Yeah so I ended up with a renderGridCell that returns an <a href={path} onClick={onCellClick}>
and

const onCellClick = event => {
    const middleMouse = event.button === 1;
    if (middleMouse) {
      event.stopPropagation();
    } else {
      event.preventDefault();
    }
  };

and then the rest is handled by onRowClick 🤷🏻

and for the css I went with

    link: {
      color: 'inherit',
      display: 'flex',
      textDecoration: 'inherit',
    },

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2021

A possible solution:

diff --git a/packages/grid/_modules_/grid/components/cell/GridCell.tsx b/packages/grid/_modules_/grid/components/cell/GridCellOwner.tsx
similarity index 96%
rename from packages/grid/_modules_/grid/components/cell/GridCell.tsx
rename to packages/grid/_modules_/grid/components/cell/GridCellOwner.tsx
index 2fba02dd..3c0fa6c0 100644
--- a/packages/grid/_modules_/grid/components/cell/GridCell.tsx
+++ b/packages/grid/_modules_/grid/components/cell/GridCellOwner.tsx
@@ -21,7 +21,7 @@ import { GRID_CSS_CLASS_PREFIX } from '../../constants/cssClassesConstants';
 import { GridAlignment, GridCellMode, GridCellValue, GridRowId } from '../../models/index';
 import { useGridApiContext } from '../../hooks/root/useGridApiContext';

-export interface GridCellProps {
+export interface GridCellOwnerProps {
   align: GridAlignment;
   className?: string;
   colIndex: number;
@@ -41,7 +41,7 @@ export interface GridCellProps {
   tabIndex: 0 | -1;
 }

-export const GridCell = React.memo(function GridCell(props: GridCellProps) {
+export const GridCellOwner = React.memo(function GridCellOwner(props: GridCellOwnerProps) {
   const {
     align,
     className,
@@ -162,7 +162,7 @@ export const GridCell = React.memo(function GridCell(props: GridCellProps) {
   });

   return (
-    <div
+    <components.Cell
       ref={cellRef}
       className={cssClasses}
       role="cell"
@@ -179,6 +179,6 @@ export const GridCell = React.memo(function GridCell(props: GridCellProps) {
       {...eventsHandlers}
     >
       {children || valueToRender?.toString()}
-    </div>
+    </components.Cell>
   );
 });
diff --git a/packages/grid/_modules_/grid/models/gridSlotsComponent.ts b/packages/grid/_modules_/grid/models/gridSlotsComponent.ts
index 16d66352..35e8f81c 100644
--- a/packages/grid/_modules_/grid/models/gridSlotsComponent.ts
+++ b/packages/grid/_modules_/grid/models/gridSlotsComponent.ts
@@ -120,6 +120,7 @@ export const DEFAULT_GRID_SLOTS_COMPONENTS: GridApiRefComponentsProperty = {
   ErrorOverlay,
   FilterPanel: GridFilterPanel,
   Footer: GridFooter,
+  Cell: GridCell,
   Header: GridHeader,
   PreferencesPanel: GridPreferencesPanel,
   LoadingOverlay: GridLoadingOverlay,

We would likely need to create a new GridCell to apply the props on a div, and accept a few "context" props. In the case of this application, we probably need to data of the cell or the row to render the link.

In practice, it would mean that:

  1. we would make [DataGrid] Add possibility to pass in component for row and cell #2138 a bit easier to solve
  2. solve [DataGrid] How to set a style base on the value of the cell (e.g. the value of the cell is the background color) #1912
  3. we give up on the idea to have renderCell/renderEditCell to render the whole cell, not only the child. https://github.com/bvaughn/react-virtualized/blob/abe0530a512639c042e74009fbf647abdb52d661/source/List/List.example.js#L200-L212

@Primajin
Copy link
Author

Yeah so that would allow passing in your own component like one is used from Composition here right?

import { Link } from 'react-router-dom';

function ListItemLink(props) {
  const { icon, primary, to } = props;

  const CustomLink = props => <Link to={to} {...props} />;

  return (
    <li>
      <ListItem button component={CustomLink}>
        <ListItemIcon>{icon}</ListItemIcon>
        <ListItemText primary={primary} />
      </ListItem>
    </li>
  );
}

Shall I open the MR for this, or would you like to? How do we go from here? 😃

@oliviertassinari
Copy link
Member

@Primajin Correct. I suspect that we would need an extra prop to host the context: the row's data, the field, the column. For this, maybe we should leverage mui/material-ui#27127.

@Primajin
Copy link
Author

Sounds good to me - I am happy to let this cook a while longer until we figure out a good and consistent way to solve it.

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!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants