mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
3.91k stars 1.19k forks source link

[data grid] onCellEditStop prop is not invoked when the edit mode is terminated in a singleSelect type column #5903

Closed TNohSam closed 1 year ago

TNohSam commented 1 year ago

Duplicates

Latest version

Current behavior 😯

When you enter EditMode for a singleSelect type column and click outside select, EditMode ends, but the event function in onCellEditStopprop is not called.

Expected behavior 🤔

Function of CellEditStopprop should be called

Steps to reproduce 🕹

  1. Define DataGrid with onCellEditStop property
  2. Add a singleSelect type column(ex: valueOptions: ['a','b'] )
  3. Press Enter or Double-click
  4. Start EditMode.(A list of choices is displayed, 'a', 'b')
  5. Click outside the list(This is important!!)
  6. EditMode ends. But, function of onCellEditStop prop is not called

Context 🔦

I have a function that I have to run every time EditMode ends. However, in the above case, Function of on CellEditStop is not called and cannot be done. And I sometimes want to keep EditMode from being modified with external clicks. However, the function is not called and cannot be done. const handleOnCellEditStop = (params, event) => { console.log("this is not called"); if (params.reason === GridCellEditStopReasons.cellFocusOut && isBrowser) { event.defaultMuiPrevented = true; } };

Your environment 🌎

    1. 25. { "dependencies": { "@emotion/react": "latest", "@emotion/styled": "latest", "@mui/icons-material": "latest", "@mui/material": "latest", "@mui/x-data-grid": "latest", "@mui/x-data-grid-generator": "latest", "@mui/x-data-grid-pro": "latest", "react": "latest", "react-dom": "latest" }, "description": "https://github.com/mui/mui-x/blob/v5.15.3/docs/data/data-grid/column-definition/ColumnTypesGrid.js", "devDependencies": { "react-scripts": "latest" } }

` const handleOnCellEditStop = (params, event) => { console.log("this is not called"); };

return ( <div style={{ height: 300, width: "100%" }}> <DataGrid columns={columns} rows={rows} experimentalFeatures={{ newEditingApi: true }} onCellEditStop={handleOnCellEditStop} />

); }`

Order ID 💳 (optional)

No response

cherniavskii commented 1 year ago

Thanks @TNohSam I can confirm onCellEditStop is not called when singleSelect menu is closed (by backdrop click or Escape key): https://codesandbox.io/s/frosty-pine-g9xced?file=/demo.tsx


SingleSelect editing component calls apiRef.current.stopCellEditMode method: https://github.com/mui/mui-x/blob/8db0bfa86ba1d4e18b78463d756eb23bddd7ec4d/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx#L152

But stopCellEditMode method doesn't publish cellEditStop event, it's the other way around - stopCellEditMode is called on cellEditStop event: https://github.com/mui/mui-x/blob/8db0bfa86ba1d4e18b78463d756eb23bddd7ec4d/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.new.ts#L197-L228

So maybe GridEditSingleSelectCell should publish cellEditStop event instead of calling stopCellEditMode? @m4theushw

m4theushw commented 1 year ago

Publishing cellEditStop seems to be the easy solution. The following diff should do this:

diff --git a/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx b/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
index 8fd7b96f3..787e21a08 100644
--- a/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
+++ b/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
@@ -149,7 +149,13 @@ function GridEditSingleSelectCell(props: GridEditSingleSelectCellProps) {
     }
     if (reason === 'backdropClick' || isEscapeKey(event.key)) {
       if (rootProps.experimentalFeatures?.newEditingApi) {
-        apiRef.current.stopCellEditMode({ id, field, ignoreModifications: true });
+        const params = apiRef.current.getCellParams(id, field);
+        apiRef.current.publishEvent('cellEditStop', {
+          ...params,
+          reason: isEscapeKey(event.key)
+            ? GridCellEditStopReasons.escapeKeyDown
+            : GridCellEditStopReasons.cellFocusOut,
+        });
       } else {
         apiRef.current.setCellMode(id, field, 'view');
       }

If the Select was not a modal, cellFocusOut would be published and fire cellEditStop automatically.

hasan-aa commented 1 year ago

When you manually call apiRef.current.stopCellEditMode, that should also publish the cellEditStop.

Currently when I listen to the onCellEditStop event and call apiRef.current.stopCellEditMode manually, the handles doesn't get called.

Edit:

I was trying to update a cell value based on update of another cell. So I thought doing it in onCellEditStophandler was a good idea. But turns out using valueSetter is a better place for this.

Regardless of this I still think my initial comment is valid.

m4theushw commented 1 year ago

When you manually call apiRef.current.stopCellEditMode, that should also publish the cellEditStop.

@hasan-aa It works in the opposite way. apiRef.current.stopCellEditMode is tied to the cellEditStop event. When the event is published, the API method is called. To have the callback called you need to publish the event like in https://github.com/mui/mui-x/issues/5903#issuecomment-1227556233

I was trying to update a cell value based on update of another cell. So I thought doing it in onCellEditStop handler was a good idea. But turns out using valueSetter is a better place for this.

You can use processRowUpdate for that.

hasan-aa commented 1 year ago

Hi @m4theushw, Thanks for pointing out processRowUpdate.

What I meant is, it should be fixed in the opposite way which is making more sense. As a user when I call apiRef.current.stopCellEditMode I expect cellEditStop to be published as a result.

m4theushw commented 1 year ago

@hasan-aa The onCellEditStop prop works as you expect in any other column type, it's only the singleSelect column that is problematic. As I mentioned in https://github.com/mui/mui-x/issues/5903#issuecomment-1227556233, if the Select component didn't use a modal the prop would be called when clicking outside. But since the modal blocks the logic to detect outside clicks, the easy way to stop the edit mode on external clicks was to explicitly call apiRef.current.stopCellEditMode. However, this decision disallowed customization from developers. To fix this behavior we only need to apply the diff from https://github.com/mui/mui-x/issues/5903#issuecomment-1227556233. Do you want to submit a PR?

To prove that cellEditStop works as expected, in other column types, take as example editing a string column type. When clicking outside the input the cellFocusOut event is published, which also publishes cellEditStop.

https://github.com/mui/mui-x/blob/d5fdc1b1fdb4a44c8157b7c5b2351a493c9bd04b/packages/grid/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts#L117-L126

Following the logic, cellEditStop calls, in the order, both the onCellEditStop prop and the apiRef.current.stopCellEditMode method (called inside handleCellEditStop).

https://github.com/mui/mui-x/blob/d5fdc1b1fdb4a44c8157b7c5b2351a493c9bd04b/packages/grid/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts#L238-L242

romgrk commented 1 year ago

@m4theushw Anything to do for this beyond applying the patch posted above?