-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 single select column type #1956
[DataGrid] Add single select column type #1956
Conversation
packages/grid/_modules_/grid/components/cell/GridEditSelectCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/cell/GridEditSelectCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/models/colDef/gridSelectOperators.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Matheus Wichman <[email protected]>
Co-authored-by: Matheus Wichman <[email protected]>
No problem, I suggested it because I had to download the PR to test it. |
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.
No problem, I suggested it because I had to download the PR to test it.
Same pain here. I think that it signals that the developers can't easily discover the full scope of this feature. How about we replace EditContractType
and EditRateType
as to simple direct application of the new column type? It might be too basic use cases to stress test the feature, but it would already be a start.
packages/grid/_modules_/grid/components/cell/GridEditSelectCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/models/colDef/gridSelectOperators.ts
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/cell/GridEditSelectCell.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Olivier Tassinari <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]>
…hub.com:DanailH/material-ui-x into feature/DataGrid-1320/create-select-column-type
type='select'
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 looks like we are almost there. Things that seem left:
- Handle
valueOptions
when is a rich object, e.g. filtering - Simplify Commodity data set with the new built-in type, e.g. EditContractType and EditRateType
packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputValue.tsx
Outdated
Show resolved
Hide resolved
{ | ||
field: 'country', | ||
type: 'select', | ||
valueOptions: ['United States', 'Germany', 'France'], |
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 if you want to render a template in the select? Ie flag and country?
Also I would have expected the grid to just generate the options by getting all the different values of a column without needing to provide those options.
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 that but what if there is a value that no column has, then it is a problem of not having a way go get it.
Regarding the template - based on that discussion #1956 (comment) we can start simple and extend it. For custom templates, the column type can be extended. I can have the same argument for the date picker and for any of the other column types.
Co-authored-by: Olivier Tassinari <[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.
In the commodity data set, I can't find one column that uses the render cell of the singleSelect
type. How about we kill renderEditCell
in field: 'rateType'
?
Otherwise, it looks good
| <span class="prop-name optional">type<sup><abbr title="optional">?</abbr></sup></span> | <span class="prop-type">string</span> | <span class="prop-default">'string'<br /></span> | Type allows to merge this object with a default definition [GridColDef](/api/data-grid/grid-col-def/). | | ||
| <span class="prop-name optional">valueFormatter<sup><abbr title="optional">?</abbr></sup></span> | <span class="prop-type">(params: GridValueFormatterParams) => GridCellValue</span> | | Function that allows to apply a formatter before rendering its value. | | ||
| <span class="prop-name optional">valueGetter<sup><abbr title="optional">?</abbr></sup></span> | <span class="prop-type">(params: GridValueGetterParams) => GridCellValue</span> | | Function that allows to get a specific data instead of field to render in the cell. | | ||
| <span class="prop-name optional">valueOptions<sup><abbr title="optional">?</abbr></sup></span> | <span class="prop-type">string \| []</span> | | To be used in combination with `type: 'singleSelect'`. This is an array of the possible cell values and labels. | |
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.
This is outdated.
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 the same as in the gridColDef
. Maybe we can say it is This is an array of the possible cell string values and/or labels.
?
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.
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.
Hmm seems to be a bug with the docs generation script. Array<string | { value: any; label: string }>
is converted to string \| []
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.
This will be fixed by #2003.
Done. I trashed a couple of more |
packages/grid/_modules_/grid/components/cell/GridEditSingleSelectCell.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.
There's an e2e test case added to test the support of portaled components, e.g. Select. We could update it to use this new column type. You have to render all columns in the full featured demo to see the columns that use the singleSelect
. I saw that you updated the maxColumns
in 0f4d127 but it was reverted.
packages/grid/_modules_/grid/components/cell/GridEditSingleSelectCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/cell/GridEditSingleSelectCell.tsx
Outdated
Show resolved
Hide resolved
I can enable it back to the full column list, not a problem. Actually even with the current setup of 20 columns, you can see the new col type (for filters for example) but the render cell component is custom. I'll bump it. |
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.
Looks good. I think you'll need to merge with master first because I merged #2003.
Sorry to close it. 😁 |
…e/DataGrid-1320/create-select-column-type
A nice addition, I was looking at the new GitHub project view WIP: https://twitter.com/github/status/1407731478096756739 and of course, they have a single select. Looks close to Airtable and Notion. |
Fixes #1320
Had to add a new prop in the
ColDef
-valueOptions
that is used to determine the select options for that column.Preview: https://deploy-preview-1956--material-ui-x.netlify.app/components/data-grid/#commercial-version