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] Implement base foundation for editing a cell #1025

Merged
merged 34 commits into from Mar 3, 2021

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Feb 10, 2021

This PR establishes the basic foundation of editability in the grid. This includes the api methods, the hook and the basic edit input component for the basic column types.

https://deploy-preview-1025--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--edit-rows-control
https://deploy-preview-1025--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--edit-rows-basic

TODO

  • fix serverside edit, control mode switch.
  • fix params passed in new handlers
  • add handlers on apiRef
  • add api description, gridOptions, api...
  • add tests
  • add docs

One iteration on #203.

@dtassone dtassone marked this pull request as draft February 11, 2021 09:53
@dtassone

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@dtassone dtassone changed the title [DataGrid] poc edit row [DataGrid] Implement base foundation for editing a cell Feb 18, 2021
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.

Once we are done with the POC. I think that the next step should be about aiming for merging, with the smallest chunk of value possible. We could break it down into smaller PRs if needed but not necessarily. I think that simply saying we have done enough for this first iteration, adding docs, tests, etc. so we can merge should do it.

I'm repeating it because the last large feature (filter) didn't go well. We have blocked merge on master for a week. Do docs, tests in different PRs. We didn't get the chance to do real code reviews, hence we couldn't self-reflect on improving the code. The first time to get the learning experience is normal, twice is not. It's important we move in small be solid steps, even if it feels slower with a short-term lense. I think that the quality of the solution should always come first, it's a great way to differentiate ourselves in this mature market :). If we have to trade, we can trade by solving fewer problems, not on the quality of the solutions.

There is still a place for YOLO, especially when saying solution X is good enough. On the process, I think that we should be rigorous.

@oliviertassinari oliviertassinari added new feature New feature or request component: data grid This is the name of the generic UI component, not the React module! labels Feb 21, 2021
@dtassone dtassone marked this pull request as ready for review February 23, 2021 17:50
@dtassone
Copy link
Member Author

Should we implement double click to edit a cell natively?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 24, 2021

Should we implement double click to edit a cell natively?

What do you mean by "should we"? Do you mean in this first pull request or in the customization demos or in the grid itself?

@oliviertassinari
Copy link
Member

It would be great to rebase, it would de-risk conflicts (it has been open for 2 weeks). It would also fix the Argos flakiness

@dtassone
Copy link
Member Author

dtassone commented Mar 1, 2021

It would be great to rebase, it would de-risk conflicts (it has been open for 2 weeks). It would also fix the Argos flakiness

I rebased many times already ;)
I will do again

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
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.

Things I have noticed, we are close:

@dtassone
Copy link
Member Author

dtassone commented Mar 2, 2021

Things I have noticed, we are close:

updateRow mutate the underlying object for perf reason. The deep freeze is just for the docs sandbox issue

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 2, 2021

updateRow mutate the underlying object for perf reason.

What do you think about taking the performance hit in (I guess it's limited since it's a single row) and gain in exchange the immutability of the data?

I assume it could create nasty bugs for the developers, for instance, if the data is shared somewhere else. We had a taste of what non-immutable data lead to with #974. I would be in favor of never allowing the data grid to mutate the args provided by developers.

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.

From what I understand, we currently need the following follow-up

  • add documentation for this new feature
  • open an issue about the capture phase event
  • improve the UI of the editable field

I also suspect that developers will complain about the edit performance, but we can wait to see. Otherwise, looks good.

@dtassone
Copy link
Member Author

dtassone commented Mar 3, 2021

From what I understand, we currently need the following follow-up

  • add documentation for this new feature

The feature is incomplete, we have to integrate the default behavior... Do we want it visible in the next release?

  • open an issue about the capture phase event

Done

  • improve the UI of the editable field

Not a blocker, can be done separately, along with the rest of the feature, what do you think?

I also suspect that developers will complain about the edit performance, but we can wait to see. Otherwise, looks good.

can be tested and fixed separately ??

@oliviertassinari
Copy link
Member

I have noticed a bug. When the edit mode is exited, the focus isn't restored to the cell.

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.

None yet

3 participants