Skip to content
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

Merged
merged 34 commits into from
Mar 15, 2022
Merged

[DataGrid] New editing API #3963

merged 34 commits into from
Mar 15, 2022

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Feb 16, 2022

This PR proposes a new API for cell and row editing. To avoid breaking changes, it's only available if experimentalFeatures.enableNewEditingAPI is true. 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.

apiRef.current.setEditCellValue({ id, field, value: '' });
apiRef.current.commitCellChange({ id, field }, event);
apiRef.current.publishEvent(GridEvents.cellEditStop, params, event);

But these published events also call API methods. Their only purpose is to allow developers to override the default behavior.

const handleCellEditStop = React.useCallback<GridEventListener<GridEvents.cellEditStop>>(
(params, event) => {
apiRef.current.setCellMode(params.id, params.field, GridCellModes.View);

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, like cellEditStart. 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 editable
  • getCellMode returns if a cell is in edit or view mode
  • startCellEditMode puts a cell into edit mode
  • stopCellEditMode puts a cell into view mode and updates the original row with the value stored
  • setEditCellValue updates the value of a cell

Methods for the row editing API

  • getRowMode returns if a cell is in edit or view mode
  • startRowEditMode puts row into edit mode
  • stopRowEditMode puts a row into view mode and updates the original row with the values stored
  • setEditCellValue updates the value of a cell

New 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 BEFORE updateRows 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 the rowEditStop 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#undoable

Methods to be removed in v6

  • setRowMode
    • Why? Replaced by startRowEditMode and stopRowEditMode
  • setCellMode
    • Why? Replaced by startCellEditMode and stopCellEditMode
  • commitRowChange
    • Why? Functionality merged into stopRowEditMode
  • commitCellChange
    • Why? Functionality merged into 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.

@mui-bot
Copy link

mui-bot commented Feb 16, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 272.8 499.3 419.4 389.3 87.646
Sort 100k rows ms 452.6 1,025.7 687.1 741.32 186.54
Select 100k rows ms 117.9 230.8 182.9 175.24 42.8
Deselect 100k rows ms 90.6 203.2 202.6 153.16 43.813

Generated by 🚫 dangerJS against dd10c21

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2022
@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request feature: Editing Related to the data grid Editing feature labels Feb 24, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 24, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 25, 2022
@@ -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;
Copy link
Member Author

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.

Copy link

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

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.

Copy link
Member

@flaviendelangle flaviendelangle left a 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.

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 5, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 7, 2022
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to add

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 8, 2022
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

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.
Copy link
Member

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.

Suggested change
* 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`

packages/grid/x-data-grid/src/models/gridEditRowModel.ts Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/models/api/gridEditingApi.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 11, 2022
@m4theushw m4theushw merged commit 140f394 into mui:master Mar 15, 2022
@m4theushw m4theushw deleted the new-editing-api branch March 15, 2022 15:28
@m4theushw
Copy link
Member Author

I merged to fix the conflicts in #4060. You can still suggest changes to the API in #4060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Editing Related to the data grid Editing feature new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants