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 useControlState hook, and add control state on selectionModel #1823

Merged
merged 39 commits into from
Jul 6, 2021

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Jun 3, 2021

Breaking changes

  • [DataGrid] Implement useControlState hook, and add control state on selectionModel ([DataGrid] Implement useControlState hook, and add control state on selectionModel #1823) @dtassone

    Normalize the controlled prop signature:

     <DataGrid
    -  onSelectionModelChange={(params: GridSelectionModelChangeParams) => setSelectionModel(params.model)}
    +  onSelectionModelChange={(model: GridSelectionModel) => setSelectionModel(model)}
     />

    Replace onRowSelected with the existing API:

     <DataGrid
    -  onRowSelected={(params: GridRowSelectedParams) =>  }
    +  onSelectionModelChange={(model: GridSelectionModel) => }
     />

https://deploy-preview-1823--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-filter--control-selection

This PR allows integrating the concept of control state, within the grid state management by registering the controlled prop, the on-change handler and the state selector which allows retrieving where the prop model is stored in state.

Below is how you can register the prop selectionModel and its onSelectionModelChange as a controlled state prop.

// You need to register a ControlStateItem to bind a model prop and onChange with its controlled state

export interface ControlStateItem<TModel> {
  stateId: string;
  propModel?: any;
  stateSelector: (state: GridState) => TModel;
  propOnChange?: (model: TModel) => void;
  onChangeCallback?: (model: TModel) => void;
}

  React.useEffect(() => {
    apiRef.current.registerControlState<GridSelectionModel>({
      stateId: 'selectionModel',
      propModel: props.selectionModel,
      propOnChange: props.onSelectionModelChange,
      stateSelector: gridSelectionStateSelector,
      onChangeCallback: (model: GridSelectionModel) => {
        apiRef.current.publishEvent(GRID_SELECTION_CHANGE, model);
      },
    });
  }, [apiRef, props.onSelectionModelChange, props.selectionModel]);

At this stage, the architecture seems to be working, and performing well.

  • implement control selection
  • add tests and stories
  • remove legacy code
  • Remove onRowSelected & GridRowSelectedParams

Breaking Change

- props.onRowSelected

Use onSelectionChange.

Next

  • RegisterControlState for filter
  • RegisterControlState for SortModel
  • RegisterControlState for pagination substates so one for page, one for pagesize
  • RegisterControlState for editRowsModel, not sure how it is going to work yet, is mapStateToModel enough 🤔
  • Cleanup, Remove redundant publishevent code, cleanup hook control mode, Fix publish event params (inconsistency btw callback and hook body)

@dtassone dtassone marked this pull request as draft June 3, 2021 16:55
@dtassone dtassone marked this pull request as ready for review June 15, 2021 10:34
@dtassone dtassone changed the title [DataGrid] Explore useControlState hook [DataGrid] Implement useControlState hook, and add control state on selectionModel Jun 15, 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.

Changing a controlled prop should directly trigger a render. Instead, it seems to be passive (wait for somebody to re-render to update). If this is true (I could have missed something in the review), then I think that we need to change the approach. found it, we have kept the effect based on the prop.

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

We have to manually call setGridState to update the state when the prop changes. Should we update the hook to also handle this update: prop -> state? I think we're gonna need a mapModelToState for that to sanitize the values that go to the state (e.g. to unselect invisible rows).

https://github.com/mui-org/material-ui-x/blob/a2a1f8eca0f8b188a1ed40abbcc616dc3a837d76/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L217-L222

@dtassone dtassone requested a review from m4theushw June 30, 2021 08:59
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@dtassone dtassone requested a review from m4theushw July 5, 2021 13:10
@dtassone
Copy link
Member Author

dtassone commented Jul 5, 2021

create single issue for the next todos

@oliviertassinari
Copy link
Member

@dtassone Could you update the PR to document the breaking changes? We are going to need it for the next release's changelog and for developers landing here. BTW, it has been your turn for the next release for the last 16 days. Are you up for making one Monday? :)

@Bixtrix
Copy link

Bixtrix commented Jul 22, 2021

@dtassone @oliviertassinari It would have been nice to write that the multiple selection (selectionModel as array input) now requires XGrid component in the release notes

Edit: Included in build v4.0.0-alpha.34

@Tai-Iro
Copy link

Tai-Iro commented Jul 22, 2021

Why is the error for multiple selections a thing? It's not mentioned anywhere in the docs or release notes, and it doesn't even stop multiple selections from working.
This change: https://github.com/mui-org/material-ui-x/pull/1823/files#diff-36939e58755c91e3693a4f18aa7796b137ba1e32f65357a940fb358286048088R247

@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 22, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 22, 2021

@Bixtrix multiple selection now requires XGrid component in the release notes
@Tai-Iro Why is the error for multiple selections a thing?

Note that multiple row selections has always been documented under the Pro plan in https://material-ui.com/components/data-grid/selection/#multiple-row-selection.

If it was possible to make work in DataGrid before with the selectionModel prop, it was a mistake from our side, sorry for the surprise. It was never intended nor documented.

Speaking of surprises, this might be also be one, not sure #1821;

@Tai-Iro
Copy link

Tai-Iro commented Jul 22, 2021

@Bixtrix multiple selection now requires XGrid component in the release notes
@Tai-Iro Why is the error for multiple selections a thing?

Note that multiple row selections has always been documented under the Pro plan in https://material-ui.com/components/data-grid/selection/#multiple-row-selection.

If it was possible to make work in DataGrid before with the selectionModel prop, it was a mistake from our side, sorry for the surprise. It was never intended nor documented.

It was directly documented as being an option in the Controlled Selection example here: https://material-ui.com/components/data-grid/selection/#controlled-selection
And every example that uses checkboxes allows selecting multiple items, of which there are several. I already mentioned it in an issue before you replied: #2193

@m4theushw
Copy link
Member

Yeah, with checkboxSelection it should allow multiple selection even with DataGrid. We missed an additional check here:

https://github.com/mui-org/material-ui-x/blob/3d1e5d8157bb12c2f27ef1223bc169890d5404db/packages/grid/data-grid/src/DataGrid.tsx#L254

Previously, there was no warning and the validation was done inside the hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants