mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.53k stars 1.32k forks source link

[data-grid] Column order state reverted when columns prop changes #7607

Open gidztech opened 1 year ago

gidztech commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example:

Steps:

  1. Define grid columns prop using factory function that relies on prop from parent
  2. Re-order columns in grid
  3. Trigger a state update that affects the prop of grid component

Current behavior 😯

I have a grid that defines the columns prop in the render of the component. This is because some of the column's renderCell handlers rely on state from a parent component. If a prop in the parent changes, I need the component rendered in renderCell to update to reflect that. In other words, the state of the cell is a combination of the data in the table and outside state.

However, this leads to the column ordering being reset when the props change. I don't have this issue where I have controlled state (e.g. sorting, selection, etc.). There is a ticket related to making it controlled here: https://github.com/mui/mui-x/issues/5704. If I make columns stateful, then the ordering doesn't get reset, but I don't get state updates when I need them.

According to the docs, the columns prop should not change, but I don't know how I can achieve what I want without it.

The columns prop should keep the same reference between two renders. The columns are designed to be definitions, to never change once the component is mounted. Otherwise, you take the risk of losing elements like column width or order. You can create the array outside the render function or memoize it.

https://mui.com/x/react-data-grid/column-definition/

Is there a way to add custom props into grid so that renderCell receives up-to-date parent state when it's called? That would allow columns to be stable perhaps.

I also looked into trying to export/restore state, by making initialState stateful, and updating it on onColumnOrderChange with the value apiRef.current.exportState(), but the changes don't seem to be reflected in grid (even if I reset the key).

https://mui.com/x/react-data-grid/state/#save-and-restore-the-state

Expected behavior 🤔

It should be possible to maintain column ordering state when column cell's re-render

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

gidztech commented 1 year ago

I have a workaround that would allow extra props to be accessed within renderCell and have columns array defined outside the component once.

Although you can pass props to componentsProps.cell, these are only accessible if you create your own custom cell renderer via the components prop (e.g. Cell: MyCellRender). I don't want to re-implement all the logic that MUI does already here, so that's not a good option. However, you can still access root props via the useGridRootProps hook. I created a cell wrapper component that gets the props object and passes it to render children.

const CellWrapper = ({ children }: { children: (props: RootCellProps) => JSX.Element | null }) => {
  const rootProps = useGridRootProps();
  const rootCellProps: RootCellProps = rootProps?.componentsProps?.cell;
  return children(rootCellProps);
};

It can be used in the renderCell function as follows:

renderCell: ({ row: { myField } }: GridRenderCellParams) => (
  <CellWrapper>
    {({ someOtherProp }) => <MyComponent field={myField} someOtherProp={someOtherProp} />
  </CellWrapper>
)

The columns array is stable now as it can be defined outside the component once.

This seems to work for me (based on my initial testing), but it would be nice if there was a way to pass the props via a documented interface that I can be confident with.

alexfauquette commented 1 year ago

I see multiple options, maybe members of the data-grid team will have better one.


Using the componentsProps.cell. In such a case, you can override the components.Cell easily, because like all the slots, the default component is exported.

Here is its definition if you wan more context about ho it works. https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/components/cell/GridCell.tsx

But basically, it wraps the result of renderCell() to add event handlers. You could pass props from componentsProps.cell to the redenderCell as follow:

const CustomCell = (props) => {
    const { mySuperProp, children, ...other } = props;
    return <GridCell {...other}>
        {React.cloneElement(children, { mySuperProp })}
    </GridCell>
}

Another solution could be to wrap the data grid in a Provider such that all your custom cells could have access to the state update without having to care about how props are propagated.

gidztech commented 1 year ago

I see multiple options, maybe members of the data-grid team will have better one.

Using the componentsProps.cell. In such a case, you can override the components.Cell easily, because like all the slots, the default component is exported.

Here is its definition if you wan more context about ho it works. https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/components/cell/GridCell.tsx

But basically, it wraps the result of renderCell() to add event handlers. You could pass props from componentsProps.cell to the redenderCell as follow:

const CustomCell = (props) => {
  const { mySuperProp, children, ...other } = props;
  return <GridCell {...other}>
      {React.cloneElement(children, { mySuperProp })}
  </GridCell>
}

Another solution could be to wrap the data grid in a Provider such that all your custom cells could have access to the state update without having to care about how props are propagated.

Thanks for the reply. The CustomCell option would work, but it's not great regarding type safety. If the component I'm rendering requires a prop, I'm now only implicitly passing it through CustomCell. TypeScript would complain of missing props because it's not passed via the interface.

It would be nicer if we were able to explicitly pass these props in a way that would expose them to the renderCell function instead of the component rendered within it. It looks like the GridRow component is what passes props to renderCell, and it only passes specific ones though:

https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/components/GridRow.tsx#L293 https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/hooks/features/rows/useGridParamsApi.ts#L115

I did consider the React Context option, but it feels a bit overkill for what is trying to be achieved. It's more natural to just be able to pass props.

alexfauquette commented 1 year ago

I did consider the React Context option, but it feels a bit overkill for what is trying to be achieved. It's more natural to just be able to pass props.

Your solution with useGridRootProps is a hacky way to use a Provider.

export const useGridRootProps = () => {
  const contextValue = React.useContext(GridRootPropsContext);

  if (!contextValue) {
    throw new Error(
      'MUI: useGridRootProps should only be used inside the DataGrid, DataGridPro or DataGridPremium component.',
    );
  }

  return contextValue as DataGridProcessedProps;
};

Could you share more context about why the cell rendering depends on an external state? Maybe it could help to get a better solution, or provide idea for a feature improvement

m4theushw commented 1 year ago

Another solution is to store in the state the order of columns and pass the columns to the columns prop according to this order. When the columns are re-ordered, you return new columns following this order. See this example: https://codesandbox.io/s/smoosh-leaf-p2p3wl?file=/demo.tsx:1931-2396

By doing this, you can have dependencies in the memoized columns as long as the their order remains the same as from the last time the user has reordered a column.

const [orderedColumns, setOrderedColumns] = React.useState([
  "id",
  "firstName",
  "lastName",
  "age",
  "fullName"
]);

const columns = React.useMemo(() => {
  const allColumns = {
    id: { field: "id", headerName: "ID", width: 90 },
    firstName: { field: "firstName", headerName: "First name" },
    lastName: { ... }
  };

  return orderedColumns.reduce((acc, field) => {
    return [...acc, allColumns[field]];
  }, []);
}, [orderedColumns]);

const handleColumnOrderChange = React.useCallback((params) => {
  setOrderedColumns((prevOrderedColumns) => {
    const newOrderedColumns = [...prevOrderedColumns];
    const oldIndex = params.oldIndex;
    const targetIndex = params.targetIndex;
    // If you have `checkboxSelection` you need to subtract 1 from both indexes above
    const oldColumn = prevOrderedColumns[oldIndex];
    newOrderedColumns.splice(oldIndex, 1);
    newOrderedColumns.splice(targetIndex, 0, oldColumn);
    return newOrderedColumns;
  });
}, []);

return (
  <Box sx={{ height: 400, width: "100%" }}>
    <DataGridPro
      rows={rows}
      columns={columns}
      onColumnOrderChange={handleColumnOrderChange}
    />
  </Box>
);

It would be nicer if we were able to explicitly pass these props in a way that would expose them to the renderCell function instead of the component rendered within it. It looks like the GridRow component is what passes props to renderCell, and it only passes specific ones though:

We'll need to discuss this more but we could add a slot only for the inner content of the cell. Something like components.cellInner.

cherniavskii commented 1 year ago

Hey @gidztech Thanks for raising this issue. This seems to be a part of a more general problem discussed in https://github.com/mui/mui-x/issues/6220 Is this correct?

gidztech commented 1 year ago

Another solution is to store in the state the order of columns and pass the columns to the columns prop according to this order. When the columns are re-ordered, you return new columns following this order. See this example: https://codesandbox.io/s/smoosh-leaf-p2p3wl?file=/demo.tsx:1931-2396

By doing this, you can have dependencies in the memoized columns as long as the their order remains the same as from the last time the user has reordered a column.

const [orderedColumns, setOrderedColumns] = React.useState([
  "id",
  "firstName",
  "lastName",
  "age",
  "fullName"
]);

const columns = React.useMemo(() => {
  const allColumns = {
    id: { field: "id", headerName: "ID", width: 90 },
    firstName: { field: "firstName", headerName: "First name" },
    lastName: { ... }
  };

  return orderedColumns.reduce((acc, field) => {
    return [...acc, allColumns[field]];
  }, []);
}, [orderedColumns]);

const handleColumnOrderChange = React.useCallback((params) => {
  setOrderedColumns((prevOrderedColumns) => {
    const newOrderedColumns = [...prevOrderedColumns];
    const oldIndex = params.oldIndex;
    const targetIndex = params.targetIndex;
    // If you have `checkboxSelection` you need to subtract 1 from both indexes above
    const oldColumn = prevOrderedColumns[oldIndex];
    newOrderedColumns.splice(oldIndex, 1);
    newOrderedColumns.splice(targetIndex, 0, oldColumn);
    return newOrderedColumns;
  });
}, []);

return (
  <Box sx={{ height: 400, width: "100%" }}>
    <DataGridPro
      rows={rows}
      columns={columns}
      onColumnOrderChange={handleColumnOrderChange}
    />
  </Box>
);

It would be nicer if we were able to explicitly pass these props in a way that would expose them to the renderCell function instead of the component rendered within it. It looks like the GridRow component is what passes props to renderCell, and it only passes specific ones though:

We'll need to discuss this more but we could add a slot only for the inner content of the cell. Something like components.cellInner.

Thanks for the response. Yep, storing the order in a controlled state like this works. There is a feature request already for having a column order model prop in MUI itself (https://github.com/mui/mui-x/issues/5704), which would make it consistent with sorting, selection etc. and require less manual manipulation.

I guess the other issue is there is other internal column state that might not be able to be controlled via props, so that would be lost if parent props change unless they are handled in the same was as above. Keeping columns stable is probably better for performance regardless of whether the column order is controlled or uncontrolled.

gidztech commented 1 year ago

Hey @gidztech Thanks for raising this issue. This seems to be a part of a more general problem discussed in #6220 Is this correct?

Yes, it's related. The code example in that thread (https://codesandbox.io/s/interesting-newton-7zr0pc?file=/demo.tsx:1100-1104) is similar to the solution I posted earlier.