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 control state/prop for all props using useGridControlState hook #2044

Closed
5 of 6 tasks
dtassone opened this issue Jul 5, 2021 · 6 comments · Fixed by #1909
Closed
5 of 6 tasks
Assignees
Labels
new feature New feature or request

Comments

@dtassone
Copy link
Member

dtassone commented Jul 5, 2021

link to #190

Summary 💡

Following core work of #1823, we now have to bind all props with their onChange handler.
Todo includes:

Examples 🌈

Motivation 🔦

Order id 💳

@dtassone dtassone added status: waiting for maintainer These issues haven't been looked at yet by a maintainer new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 5, 2021
@dtassone
Copy link
Member Author

dtassone commented Jul 7, 2021

FYI I'm picking up control sort model

@flaviendelangle
Copy link
Member

flaviendelangle commented Jul 7, 2021

For the pagination

The filterModel is a single prop in GridOptions, which correspond to the filter key of the state.
But for the pagination, we don't have this equivalent, we have several options that either can or can't be controlled.

Descriptions of the rows

  • paginationMode which is coming from a prop (with a default fallback)
  • rowCount which is coming from a prop if paginationMode="server" or from the rows (in which case it's a derived state)
  • pageCount which is computed from the other values of the pagination state in applyConstraints

Descriptions of the current pagination

  • pageSize
  • page

For this part, we can have a controlled / uncontrolled behavior.
But unless the filterModel, these are two props, not a single paginationModel

<Grid page={0} pageSize={20} />

Instead of

<Grid paginationModel={{ page: 0, pageSize: 20 }} /> 

To migrate to a behavior closer to the other controlled model, I propose to :

  • Replace the page and pageSize options by a paginationModel?: { page: number, pageSize: number } which will be stored in the pagination state

If we do not want to do this breaking change, I can keep the 2 props and do a little bit a magic in onChangeCallback to decide which event to call. But this mean that the user can still give the page but not the pageSize and be in a semi-controlled mode, which is not great.

  • Create a useRowCount hook that returns the option rowCount if defined or the result of visibleGridRowCountSelector (this is the current behavior but right now the result is stored in the state in a useEffect, causing incoherent intermediate states (see [DataGrid] Setting page with ApiRef in useEffect does not work anymore #1815 (comment))

  • Create a usePageCount hook that returns the pageCount computed with useRowCount and state.pagination.pageSize.

  • Remove paginationMode from the state and just add a default value "client" in useOptionsProp

@dtassone
Copy link
Member Author

dtassone commented Jul 7, 2021

@flaviendelangle you just need to bind the props with their onChange events
so page with onPageChange , and pageSize with onPageSizeChange.
Then refactor the handlers to just take the props

  onPageChange?: (page) => void;

  onPageSizeChange?: (pageSize) => void;

We don't need to control every prop, just the ones that have an onChange handler.

@flaviendelangle
Copy link
Member

flaviendelangle commented Jul 7, 2021

So you would do something like that ?

  React.useEffect(() => {
    apiRef.current.updateControlState<number>({
      stateId: 'page',
      propModel: props.page,
      propOnChange: props.onPageChange,
      stateSelector: (state) => state.pagination.page,
      onChangeCallback: (model) => {
        apiRef.current.publishEvent(GRID_PAGE_CHANGE, model);
      },
    });
  }, [apiRef, props.page, props.onPageChange]);

  React.useEffect(() => {
    apiRef.current.updateControlState<number>({
      stateId: 'pageSize',
      propModel: props.pageSize,
      propOnChange: props.onPageSizeChange,
      stateSelector: (state) => state.pagination.pageSize,
      onChangeCallback: (model) => {
        apiRef.current.publishEvent(GRID_PAGE_SIZE_CHANGE, model);
      },
    });
  }, [apiRef, props.pageSize, props.onPageSizeChange]);

An issue I see with these 2 updateControlState is that we will sometime want to update both page and pageSize in the state at once.
For instance, in the useGridPagination we have a single useEffect that tracks options.paginationMode, visibleRowCount, options.rowCount, options.autoPageSize, containerSizes?.viewportPageSize, options.pageSize, options.page). And with 2 stateId impacted by a single state update, I get this error You're not allowed to update several sub-state in one transaction as intended.

I can avoid updating the two variables together, but it makes a strange code : https://github.com/mui-org/material-ui-x/pull/2099/files#diff-88ab07d0295dc05cfb45a167032079961737adbcc04e547dafd7762f817bf5d7R93
I have to put 3 effets to update both variables and then apply the constraints on the state.


This won't fix this
But I suppose we can solve the two problem separably.
I understood on the monday meeting that the control state/prop rework was supposed to solve this problem.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 31, 2021

I'm closing as we have migrated all the controlled props, well-done @dtassone, @m4theushw, @flaviendelangle.

@zubozrout
Copy link

zubozrout commented Aug 16, 2021

Hello and apologize for spamming here,
however this new behaviour has made quite an impact on my little project and I am not sure how to workaround the server-side sorting with the new controlState sortModel.

I can give DataGrid anything, even a manually filled in sortModel. That works like a charm, but now when onSortModelChange calls my function that passes new settings to server it only briefly shows the respective data server returned and then in a blink of an eye reverts back to the original hard-coded sortModel that DataGrid has on load.

I also had a look at the "Story" tab here https://deploy-preview-2099--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-sorting--server-side-sorting&globals=measureEnabled:false but sadly that didn't help me at all. If it is even relevant, not sure.

Could you please give me a very brief example/a few hints on how this is supposed to work now? That is how am I supposed to change the sortModel itself properly? Even if I have it as a state variable overwriting it inside onSortModelChange callback to whatever DataGrid returns only ends up with a never-ending loop.

Thank you very much for all your help.

EDIT: I have my answer now and it was enough to see this updated documentation https://material-ui.com/components/data-grid/sorting/ which is quite self-explanatory. So please disregard this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants