Open m4theushw opened 2 years ago
@mui/x I would like your opinion on this one.
I think memoizing columns with a deps like this is a bad pattern because it causes a re-computation of the whole grid (sorting / filtering / aggregation / grouping / column processing etc...) for something that should only impact two rows (the one being un-hovered and the one being hovered)
For me the clean implementation on the user side would be with a React context and no reset of the columns whatsoever. https://codesandbox.io/s/data-grid-community-forked-8zg7wp?file=/src/App.tsx
I feel like your 4 solutions adds a non-negligible amount of complexity
There may be use-case where users do have to put deps in the column useMemo
. But I did not see any yet.
Right now we have a behavior that I like:
If we do open the apiRef on the community plan (#4070 has some issues, I think we can open it to the community without the public / private split by the way), then we could keep this distinction.
Because you have the same problem for row reordering If you reorder then hover a cell, you loose the order
And putting more and more properties of the columns outside of there definition is not great I think.
@flaviendelangle But then it means that the columns should never change and all possible columns should be defined a priori. This is not always possible. For instance, if the columns are dynamic and change based on the content displayed (e.g. columns extracted from the rows
prop), they may change when viewing another page. #3200 loads columns from an API. I agree that columns
shouldn't relay on many props, but only a single prop as dependency already breaks column resizing.
the columns should never change and all possible columns should be defined a priori.
No, it would mean that for dynamic columns, people should use apiRef.current.updateColumns
whenever possible.
I indeed think that changes in props.columns
should be as rare as possible and that our main goal should be to provide API to avoid having users relying on this change in the 1st place.
Because by definition, updating this prop is heavy. We can try to change the way some feature behave to have them work with props.columns
updates, but I think in general we should teach users how to update there columns without updating this prop.
If you want to support your example with the deps in the useMemo
, would you also extract the column order outside of props.columns
?
I like solution no.1, mostly because it allows to define columns dynamically instead of using updateColumns
(which is kind of the point of React - declarative over imperative) while avoiding width reset after resizing the columns
That's a pretty neat solution, @flaviendelangle ! We use MUI at work and had the same column-sizing-auto-resets issue. I started off with your createContext()
approach, which did the trick.
However, I explored the useRef()
approach (perhaps the same as apiRef
that you mentioned above) and that seemed to be a better fit overall, so I just thought of sharing the idea with you all:
const MyAwesomeTable = ({ rawRowData }) {
/*
* + backup of row data
* + will not be re-computed across re-renders
* + useful when row data can be filtered (such as during a search)
* - changes in original row data won't be picked up without a component re-mount
*/
const _rawRowData = useRef(rawRowData);
/*
* + The exact `DataGrid` row action item's reference (such as an edit row "pen icon")
* + Stored like this: { id1: Element1, id2, Element2, ... } for each row
* - Loop will run everytime the component is re-evaluated/rendered
*/
const actionNodeRefs = {};
_rawRowData.current.forEach(row => actionNodeRefs[row.id] = useRef(null));
/*
* + stateful column information not re-computed across re-renders
*
* column "actions" contains a "pen icon" that must appear when a row is hovered
* `ref=...` attaches a reference to this node at `actionNodeRefs[rowId]`
* initially do not show the icon
*/
const [columns] = useState([
{
field: 'first_name',
headerName: 'Firstname',
...
},
...
{
field: 'actions',
type: 'actions',
/*
* edit "pen icon"
*/
getActions: (params) => {
return [
<GridActionsCellItem
label="Edit"
icon={ <MyAwesomePenIcon /> }
ref={actionNodeRefs[params.id]} // attach a reference to this node
style={{ display: 'none' }} // initially it's hidden as no row is hovered
/>,
];
},
...
},
]);
...
<DataGridPremium
...
slotProps={{
row: {
onMouseEnter: (e) => {
/*
* show the "pen icon"
*
* + `data-id` attribute is provided by `DataGrid` on all rows
* + points to the row id in question
*/
actionNodeRefs[e.currentTarget.getAttribute('data-id')].current.style.display = 'block';
},
onMouseLeave: (e) => {
/*
* hide the "pen icon"
*/
actionNodeRefs[e.currentTarget.getAttribute('data-id')].current.style.display = 'none';
}
}
}}
/>
}
Problem
Given we have a memoized array of column definitions with explicit widths, e.g.
width: 200
, if one of the columns is resized and the reference of the array is changed, the widths of the columns are reverted to the sizes specified in the definition. One issue that implemented these conditions is https://github.com/mui/mui-x/issues/4686. Although the problem in it can be fixed with CSS, I think we should consider that the column definitions may change.Example:
If an explicit width (uses the default column width) is not defined it works correctly. This happens because
newColumn.width
doesn't exist, not overridingexistingState.width
. If there's a width,newColumn.width
always win.https://github.com/mui/mui-x/blob/e5b548f6744f6d000436879839067055205e07db/packages/grid/x-data-grid/src/hooks/features/columns/gridColumnsUtils.ts#L397-L402
Proposed solution
Rename
GridColDef.width
toGridColDef.initialWidth
.This solution transmits to the user the idea that the width passed may not be the final one, but will be used initially.
GridColDef.width
can still be used, but will always win even if the column is resized.https://github.com/mui/mui-x/blob/e5b548f6744f6d000436879839067055205e07db/packages/grid/x-data-grid/src/hooks/features/columns/gridColumnsUtils.ts#L197
Use the existing
computedWidth
above.I didn't investigate all side effects. One I can think immediately is that
computedWidth
will be used even if the user changesGridColDef.width
. Not sure if it works.Create
columnDimensionsModel
propWe move to this prop all the dimension attributes. In parallel to it, we add the
onColumnDimensionsModelChange
callback prop, called when the user resizes one column. This solution is similar tocolumnVisibilityModel
. It will be a pain to, temporarily, keep supporting the dimension attributes inside theGridColDef
so it might be better to implement this only in v6.Only add a demo supporting resizing and explicit widths
We could listen to the
columnResize
event and store in an object the new width of the column. In the column definition, the width from the object is used. It's more or less whatonColumnDimensionsModelChange
does, but in a rudimentar way.