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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Improve API to control cell editing through custom components #1824

Closed
1 task done
m4theushw opened this issue Jun 3, 2021 · 2 comments 路 Fixed by #1955
Closed
1 task done

[DataGrid] Improve API to control cell editing through custom components #1824

m4theushw opened this issue Jun 3, 2021 · 2 comments 路 Fixed by #1955
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@m4theushw
Copy link
Member

m4theushw commented Jun 3, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 馃挕

While I was creating the components for #1817 I had some problems with the cell editing API. I'm listing all of them here because I think it demands work in this entire API and not just in a particular method.

  1. Inconsistencies on how to save values - I first raised this in [DataGrid] Add valueParser to parse values entered by the user聽#1785 (comment) but I faced the same problem in [docs] Make cell editable in demos聽#1817. We have different components using different APIs to do the same thing. Officially, users should use commitCellChange but this same method has its problems. Which API both us and developers should use to create custom edit components?

    • GridEditInputCell and GridEditBooleanCell publish the GRID_CELL_EDIT_PROPS_CHANGE event
    • The ValidateServerNameGrid demo calls setEditCellProps
    • The demo with the rating component calls commitCellChange.
  2. Calling commitCellChange without setCellMode reverts the value when clicking outside the cell - Open this CodeSandbox, change a value in the favorite color column and click outside the cell to save. It reverts to the original value, but it shouldn't. The reason to not call setCellMode would be if I want to stay in the edit mode after selecting a value (e.g. a Slider).

  3. Keyboard support? - Open this CodeSandbox and select a new value for a cell of the favorite color column with the keyboard. When Enter is pressed it doesn't change to the view mode, even using setCellMode. In [docs] Make cell editable in demos聽#1817 I had to call setEditCellProps when the value is selected through the keyboard.

  4. Components that use a backdrop - Since I changed in [DataGrid] Add support for edit components that use portal聽#1772 to relay in the click, instead of blur, we now have a new problem. If a component has a backdrop, clicking "outside" it doesn't exit from the edit mode because for us the click was inside it. That being said, we should provide a new API so users can handle the outside click themselves. That means publishing GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED and GRID_CELL_EDIT_EXIT.

    https://github.com/mui-org/material-ui-x/blob/b1aac5cd14d3e5bba44cdc612dcf35a78c0a50d8/packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts#L65-L71

@m4theushw m4theushw added status: waiting for maintainer These issues haven't been looked at yet by a maintainer component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 3, 2021
@dtassone
Copy link
Member

dtassone commented Jun 4, 2021

The ValidateServerNameGrid demo calls setEditCellProps

Yes this is to do onChange validation and continue editing.

The demo with the rating component calls commitCellChange

This one changes the value and commits straightaway back to view mode. When you select a rate, ie nb of stars, you click and you're done. This is the reason why it works this way.

Hence why they are different.

  1. should work as rating. When you change an input, you stay in edit on change. Hence why the behavior is different

  2. seems like a regression issue or a bug

  3. As I mentioned before, we should have a usePortals to subscribe to the portal events and act accordingly

@m4theushw
Copy link
Member Author

m4theushw commented Jun 4, 2021

@dtassone Shouldn't we split the functionality of each method and encourage one approach only in both the predefined components and demos?

Based on the name of the methods, I would expect that they would work like the git: first a file is moved to the staging and from there it can be commited. This applied to our API means:

  • setEditCellProps - saves the new value to the state
  • commitCellChange - gets the value stored in the state and copies it to the cell

Regarding the demo with the rating component, why not to use setEditCellProps on it?

As I mentioned before, we should have a usePortals to subscribe to the portal events and act accordingly

Could you describe in detail what this new hook would do? We had this discussion in #1772 (comment) but I couldn't see why we need to abstract this logic. The problem I related in 4 is not exclusive to portals but to any component that has a backdrop. Even using the blur, the problem would occur. In the future, with the new Select component that doesn't use backdrop, we can use the ClickAwayListener here.

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! new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants