-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] List View #14393
[DataGrid] List View #14393
Conversation
Deploy preview: https://deploy-preview-14393--material-ui-x.netlify.app/ Updated pages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love that approach ... definitely worth exploring this a bit more IMO. 💙
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Kenan Yusuf <[email protected]>
packages/x-data-grid/src/hooks/features/listView/useGridListView.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/listView/useGridListView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation behind having the list view column definition in the state?
I'm not saying it's wrong, but I'm curious if there are any benefits to it compared to consuming it from the prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a few places, e.g. useGridVirtualScroller
and useGridScroll
, we need all of the visible columns. In some of those places it is expecting the visible columns to have a computedWidth
property, which doesn't exist on the list column definition by itself. Before we were just calculating this at the point of rendering the cell (in GridRow.tsx), but it made sense to me that we calculate it once and store it in state so that every time we need the list of visible columns, it has the computedWidth
. Perhaps there is another way to satisfy this that I didn't consider...
Co-authored-by: Andrew Cherniavskii <[email protected]> Signed-off-by: Kenan Yusuf <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not having this as a new data set in the generator?
return [ | ||
...data.columns, | ||
{ | ||
type: 'actions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is strange that using list view and list column you get actions still from the regular columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cherniavskii thoughts on this? I wonder if listColumn
should become listColumns
, and we require users to specify actions for list view and regular view separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this too. It totally makes sense for the actions column use case, especially for the ListViewAdvanced
demo.
But should we limit the number of items in listColumns
to 2? And how do we distribute the width between them? Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided not to persist the actions column, and make actions part of the list view cell instead.
This way, the list view cell always takes 100% of the width.
Done in 034c355
(#14393)
*/ | ||
export type GridListColDef<R extends GridValidRowModel = any, V = any, F = V> = Pick< | ||
GridBaseColDef<R, V, F>, | ||
'field' | 'renderCell' | 'align' | 'cellClassName' | 'display' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why these are being picked up.
in the example only field
and renderCell
are being used.
is there plan to support this in a more out-of-box way where you might not need custom renderer for simple use cases?
}); | ||
}; | ||
|
||
const prevInnerWidth = React.useRef<number | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to initialize the ref with a number value instead? It makes the typing more simple, and keeps code paths that use this value monomorphic. -1
seems like it could be a good candidate. If this value is sometimes a float, then NaN
can also be a good initial value.
Preview: https://deploy-preview-14393--material-ui-x.netlify.app/x/react-data-grid/list-view/
Based on https://www.notion.so/mui-org/x-grid-Mobile-view-for-Data-Grid-b6228123c8a64352ba2c355c46e4e4ff
Also fixes #14922
TODO
Not doing
Row reorderingCheckbox selectionShould Export have a special treatment in mobileView?Master detailFollow up issues