-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] New editing API #3963
Conversation
These are the results for the performance tests:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2b6764a
to
2e7f536
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
114e9e0
to
1b82f63
Compare
packages/grid/x-data-grid-pro/src/tests/detailPanel.DataGridPro.test.tsx
Outdated
Show resolved
Hide resolved
@@ -74,6 +75,10 @@ function GridEditSingleSelectCell(props: GridRenderEditCellParams & Omit<SelectP | |||
setOpen(false); | |||
const isValid = await api.setEditCellValue({ id, field, value: event.target.value }, event); | |||
|
|||
if (rootProps.experimentalFeatures?.enableNewEditingAPI) { | |||
return; |
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.
The edit cell in the current singleSelect
column automatically commits the value after selecting an option. This doesn't work well with preProcessEditCellProps
because if doing a AJAX validation it freeze the UI while waiting for the server to respond. Now we don't automatically commit the value. The user has to click outside as in any other column.
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.
If preProcessEditCellProps
is not being used and/or the desired UX is that once a user has made a MenuItem
selection the singleSelect
will automatically commit, is there a prescribed method for doing so? Or, is there a workaround to make that happen? Thank you.
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.
Based on the auto-stop example, my understanding is this cannot be done with the singleSelect
column type but rather must be accomplished with a custom column type. While the custom column type may be a Select
, it would not be a singleSelect. Thank you.
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 did not test the behavior yet
Overall, the new API is a lot easier to read.
packages/grid/x-data-grid/src/internals/models/api/gridEditingApi.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/internals/models/api/gridEditingApi.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/internals/models/api/gridEditingApi.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/internals/models/params/gridCellParams.ts
Outdated
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
Nothing to add
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@@ -670,4 +674,11 @@ export interface DataGridPropsWithoutDefaultValue extends CommonProps { | |||
* For each feature, if the flag is not explicitly set to `true`, the feature will be fully disabled and any property / method call will not have any effect. | |||
*/ | |||
experimentalFeatures?: Partial<GridExperimentalFeatures>; | |||
/** | |||
* Callback called before updating a row with new values in the row and cell editing. |
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.
Users not looking at the doc but only at there IDE autocomplete will probably use it without thinking to enable the flag.
* Callback called before updating a row with new values in the row and cell editing. | |
* Callback called before updating a row with new values in the row and cell editing. | |
* Only applied if `props.experimentalFeatures.newEditingApi: true` |
This PR proposes a new API for cell and row editing. To avoid breaking changes, it's only available if
experimentalFeatures.enableNewEditingAPI
istrue
. Since we switch hooks, developers can't change which API to use in runtime.After working a lot with the current API it seems to me that during development the listeners were created before the API methods. This caused weird things like publishing events and also calling other API methods.
mui-x/packages/grid/x-data-grid/src/internals/hooks/features/editRows/useGridCellEditing.ts
Lines 228 to 230 in 3101d8d
But these published events also call API methods. Their only purpose is to allow developers to override the default behavior.
mui-x/packages/grid/x-data-grid/src/internals/hooks/features/editRows/useGridCellEditing.ts
Lines 299 to 301 in 3101d8d
This was making the editing hooks a mess to maintain. To fix these problems I took a different direction here. First I created the API methods, then binded them to events. Generic listeners, like
cellKeyDown
, are dumb now and they will only be used to publish a specific editing event, likecellEditStart
. The specific event listeners will also be dumb, only calling a method from the API with the correct params. All the complex logic will be inside the API method.Methods for the cell editing API
isCellEditable
determines if a cell is editablegetCellMode
returns if a cell is in edit or view modestartCellEditMode
puts a cell into edit modestopCellEditMode
puts a cell into view mode and updates the original row with the value storedsetEditCellValue
updates the value of a cellMethods for the row editing API
getRowMode
returns if a cell is in edit or view modestartRowEditMode
puts row into edit modestopRowEditMode
puts a row into view mode and updates the original row with the values storedsetEditCellValue
updates the value of a cellNew props
processRowUpdate
(name open for discussion) is called when putting a cell/row back to view mode. It's meant to be used to send the new row to the server. Since it's called BEFOREupdateRows
it allows developers to also pre-process the row that will be saved. If the promise is rejected, then we don't update the row and the row doesn't goes to view mode. Previously, to send the row to the server we taught developers to use therowEditStop
event but this had a problem that it updated the row even when the server threw an error, it was optimistic. Now we use a pessimistic approach, which IHMO is what users want for a CRUD application. See https://marmelab.com/react-admin/CreateEdit.html#undoableMethods to be removed in v6
setRowMode
startRowEditMode
andstopRowEditMode
setCellMode
startCellEditMode
andstopCellEditMode
commitRowChange
stopRowEditMode
commitCellChange
stopCellEditMode
Demo with server-side validation: https://codesandbox.io/s/validateservernamegrid-material-demo-forked-5iflei?file=/demo.tsx
Demo with server-side persistence: https://codesandbox.io/s/celleditserversidepersistence-material-demo-forked-7xcpgg?file=/demo.tsx (it doesn't save if the name is empty)
Demo with row editing, validation and with the default start/stop editing behaviors disabled https://codesandbox.io/s/starteditbuttongrid-material-demo-forked-y84q5e?file=/demo.tsx
The docs will be done in a follow-up PR.