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] Add row editing feature #2098

Merged
merged 18 commits into from
Aug 17, 2021
Merged

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Jul 7, 2021

Closes #204
Based on #2087
Previews: https://deploy-preview-2098--material-ui-x.netlify.app/components/data-grid/editing/#row-editing

Based on the benchmarks, I think that the easiest path to implement this feature is to leverage the current cell editing, but with the difference that the value is committed once for the entire row, instead of on each cell change. There's already an editMode prop which is not being used, so I'm repurposing it to be used to choose between cell and row.

TODO

  • Tests
  • Update docs
  • Add onEditRowChangeCommit prop
  • Update edit components to support the row editing (select shouldn't open straightaway)

@m4theushw m4theushw force-pushed the row-editing branch 2 times, most recently from 2c4f32c to ae65c54 Compare July 8, 2021 23:09
@m4theushw m4theushw added the on hold There is a blocker, we need to wait label Jul 14, 2021
@m4theushw m4theushw force-pushed the row-editing branch 4 times, most recently from 0755d87 to 25ed1c9 Compare August 9, 2021 12:58
@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 and removed on hold There is a blocker, we need to wait labels Aug 9, 2021
@m4theushw m4theushw added this to the Sprint 20 milestone Aug 9, 2021
@m4theushw m4theushw marked this pull request as ready for review August 9, 2021 13:28
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks like an awesome starting point!

@m4theushw
Copy link
Member Author

On this demo https://deploy-preview-2098--material-ui-x.netlify.app/components/data-grid/editing/#conditional-validation, the state is lost, sometimes

@oliviertassinari I couldn't reproduce it consistently, it happened almost randomly here. I suppose it loses the state because the time between clicks is too small. There's the first click, the internal state is updated with the new value, then it calls onEditRowsModelChange which also causes a rerender, but when it's called the second click already happened. I propose to debounce in a second PR the value update of each edit component:

https://github.com/mui-org/material-ui-x/blob/aa637c3eabcfad097b2c59af5f4efcf4041080d3/packages/grid/_modules_/grid/components/cell/GridEditBooleanCell.tsx#L38

If you open this other demo in local host https://deploy-preview-2098--material-ui-x.netlify.app/components/data-grid/editing/#client-side-validation, the UX of erasing the content of the text input is not good. It's lagging because there's a rerender each time a character is pressed.

Comment on lines 77 to 78
[`${GRID_CSS_CLASS_PREFIX}-row--editing`]: apiRef.current.getRowMode(id) === 'edit',
[`${GRID_CSS_CLASS_PREFIX}-row--editable`]: editMode === 'row',
Copy link
Member

Choose a reason for hiding this comment

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

Should we move those in the classes constant? Or better yet with GridClasses from here #2320

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll keep as it is, then after #2320 I change to use gridClasses.

@DanailH
Copy link
Member

DanailH commented Aug 11, 2021

I tested it and it is an awesome feature 🚀.

The only thing I was able to find is when you add new rows and the rows exceed the overflow of the window you are not automatically scrolled to the newly created row. It makes sense for the user to get scrolled to that row since when you create it it is in edit mode. The same happens when you use pagination and you have a pageSize=5 and you have 5 rows but then you add another one, which goes to the next page - it makes sense for the pagination to open the second page.

@m4theushw
Copy link
Member Author

The only thing I was able to find is when you add new rows and the rows exceed the overflow of the window you are not automatically scrolled to the newly created row.

Now it's scrolling to the new row.

The same happens when you use pagination and you have a pageSize=5 and you have 5 rows but then you add another one, which goes to the next page - it makes sense for the pagination to open the second page.

Interesting, we could investigate how to make scrollToIndexes support pagination.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have personally seen enough. Happy to go to production with the current version. I have left more comments to polish. Great work, well done!

On this demo https://deploy-preview-2098--material-ui-x.netlify.app/components/data-grid/editing/#conditional-validation, the state is lost, sometimes

I have found it using console.warn to get the stack track. It wasn't easy, took me half an hour! The reset happens when the double click triggers. I have pushed a commit 35df73f that replicates the same logic of setCellMode in setRowMode. We can skip the whole section if we are already in edit mode. Fixed

It's lagging because there's a rerender each time a character is pressed.

I remember making the same point to Damien in a PR"s review. One possible solution could be to keep the state of the input local, and only sync with the parent data grid on commits.

We could investigate how to make scrollToIndexes support pagination.

We could, maybe we should wait for a developer to complain?

@oliviertassinari oliviertassinari modified the milestones: Sprint 20, Sprint 21 Aug 16, 2021
@m4theushw m4theushw merged commit 4a20189 into mui:master Aug 17, 2021
@m4theushw m4theushw deleted the row-editing branch August 17, 2021 17:41
@oliviertassinari
Copy link
Member

🎊

@oliviertassinari oliviertassinari changed the title [DataGrid] Row editing [DataGrid] Add row editing feature Aug 24, 2021
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 this pull request may close these issues.

[DataGrid] Implement Row editing
4 participants