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

[core] Remove state.isScrolling #2337

Merged
merged 4 commits into from
Aug 26, 2021
Merged

[core] Remove state.isScrolling #2337

merged 4 commits into from
Aug 26, 2021

Conversation

m4theushw
Copy link
Member

One of #820

@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 labels Aug 13, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Remove state.isScrolling [core] Remove state.isScrolling Aug 14, 2021
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Aug 14, 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.

One of #820

For this issue, I'm not sure that we need to remove things in the state. Would it work if we segment it instead? Meaning, have different top-level sections:

  • state: the root
    • public: the part that developers can store in their DB
    • private: where we store private things

Another alternative is to add a getPublicState method that returns a subset. One downside with this issue is that it doesn't force us to think upfront about what's public and what's private. It's more or less what AG Grid for instance, by forcing developers to cherry-pick what they want to store: https://ag-grid.com/react-grid/column-state/.

If we think about it, what would developers want to store? Based on the duplicates of #820:

  • Column reorder
  • Column resized
  • Column visibility
  • Density mode applied
  • Filter applied
  • Sorting applied

Is there anything else? I don't see it.

Comment on lines 65 to 66
useGridApiEventHandler(apiRef, GridEvents.columnResizeStart, handleColumnResizeStart);
useGridApiEventHandler(apiRef, GridEvents.rowsScroll, handleRowsScroll);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to simplify even futher?

Suggested change
useGridApiEventHandler(apiRef, GridEvents.columnResizeStart, handleColumnResizeStart);
useGridApiEventHandler(apiRef, GridEvents.rowsScroll, handleRowsScroll);
useGridApiEventHandler(apiRef, GridEvents.columnResizeStart, hideColumnMenu);
useGridApiEventHandler(apiRef, GridEvents.rowsScroll, hideColumnMenu);

@m4theushw
Copy link
Member Author

For this issue, I'm not sure that we need to remove things in the state.

@oliviertassinari I think we have two things to have in mind here. The first one being to solve #820 and another one which is the opportunity to clean the state. Besides the first one, removing data not widely used from the state might be worth it to reduce complexity. Now we have a good view of what kind of information must be in the state and what's used only in one hook. For everything that is not shared between hooks, we could use internal state.

Would it work if we segment it instead? Meaning, have different top-level sections:

My idea would be to have a single state. Having a private state seems that we're storing derived data. With a single state, developers only need to serialize the state and save in their DBs. To restore the session, we just replace the entire state with the newer one, doing some sanity check before (e.g. a column might not exist anymore).

If we think about it, what would developers want to store? Based on the duplicates of #820:

I can also think about pagination, selection and with features like #210 we might want to store other stuff (opened/closed nodes).

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 22, 2021

@m4theushw Cleaning the state sounds great.

For the "private state", this option is to use the public state as the source of truth and to use the private state to store intermediary or short-lived state there, stuff that could be recomputed at any time, the same way of how useMemo behaves "You may rely on useMemo as a performance optimization, not as a semantic guarantee.".

@flaviendelangle
Copy link
Member

Yeah it's a great idea to remove this kind of data from the state, it will help a lot when trying to change the implementation of the state.

@m4theushw m4theushw changed the base branch from master to next August 26, 2021 14:47
@m4theushw m4theushw merged commit 7f25b1d into mui:next Aug 26, 2021
@m4theushw m4theushw deleted the clean-state branch August 26, 2021 16:48
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! core Infrastructure work going on behind the scenes new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants