[data grid] Move conditional list view column logic into gridVisibleColumnDefinitionsSelector
#18724
Conversation
|
Deploy preview: https://deploy-preview-18724--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+46B(0.00%) - Total Gzip Change: ▼-116B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-data-grid parsed: 🔺+23B(+0.01%) gzip: ▼-36B(-0.03%) |
gridVisibleColumnDefinitionsSelector
gridVisibleColumnDefinitionsSelectorgridVisibleColumnDefinitionsSelector
| @@ -391,7 +391,7 @@ | |||
| }, | |||
| { | |||
| "name": "gridVisibleColumnDefinitionsSelector", | |||
| "returnType": "GridStateColDef[]", | |||
| "returnType": "(GridListViewColDef & { computedWidth: number })[]", | |||
There was a problem hiding this comment.
This doesn't look right.
If anything, it should be GridStateColDef[] | GridListViewColDef[].
There was a problem hiding this comment.
Hmm yes, it's returning the intersection type that can represent both return values. We could manually assert the return type of the selector to GridStateColDef[] | GridListViewColDef & { computedWidth: number }[]... not sure if there's a better option?
There was a problem hiding this comment.
Does GridListViewColDef fit inside GridStateColDef? We've been storing the output of that logic in variables that act as GridStateColDef[] (edit: or is it GridColDef[]?), in theory we should be able to keep the type as GridStateColDef[].
There was a problem hiding this comment.
GridListViewColDef is more narrow than GridStateColDef
There was a problem hiding this comment.
If it is not possible to make the type compatible, you can create a new selector and deprecate this one
otherwise, everyone getting the new grid version and using this selector will have to explicitly set the type as you did in our code
example
-(col) => col.flex && col.flex > 0,
+(col: GridStateColDef) => col.flex && col.flex > 0,There was a problem hiding this comment.
I think we can't change the return type of this selector.
How about a hack, where in listView mode we return GridListViewColDef enhanced with a bunch of fields from GridStateColDef set to undefined? If that's possible, we can keep the return type as GridStateColDef[].
There was a problem hiding this comment.
All the fields outside of the GridListViewColDef and GridStateColDef intersection already seem to be optional (the only required coldef field is field: string), so I think we could just do a cast, no need to add actual undefined values. The only problem is computedWidth: number. I would add a selector like gridListViewStateColDef to transform the listview column to a normal GridStateColDef (even using the proper computedWidth, which would be dimensions.root.width I think). Tbh that's how I would have implemented listView to start with, just turn it into a normal column and let the existing logic work as before.
There was a problem hiding this comment.
I think we could just do a cast, no need to add actual undefined values
Right 👍🏻
The only problem is computedWidth: number. I would add a selector like gridListViewStateColDef to transform the listview column to a normal GridStateColDef
There already is gridListColumnSelector which returns GridListViewState:
GridListViewState = (GridListViewColDef & { computedWidth: number }) | undefined
So casting this one totally makes sense to me 👍🏻
packages/x-data-grid/src/hooks/features/keyboardNavigation/useGridKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
| @@ -27,8 +27,8 @@ export interface GridPinnedColumnFields { | |||
| } | |||
|
|
|||
| export const EMPTY_PINNED_COLUMN_FIELDS = { | |||
| left: [] as string[], | |||
| right: [] as string[], | |||
There was a problem hiding this comment.
Casting as string[] changed the gridVisiblePinnedColumnDefinitionsSelector return type since that can now return empty pinned columns when in list view — I don't think it is needed, but please let me know if you know different.
Part of #14997
Moves list view conditional column rendering logic to the visible column selector.