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] Replace prop.state with prop.initialState #2848

Merged
merged 17 commits into from
Oct 19, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 12, 2021

I only implemented the initial states that were used in our tests, docs or stories (filter, sorting and preferencePanel), I can implement others (pagination, selection, density) but I think starting small is not a bad idea.

Goals

Remove prop.state without loosing the ability to open the preference panel on mount

The current prop.state is dangerous to use on most sub-states because it forces the user to also pass the internal values (for instance the sortedRows for state.sorting).
Moreover, if we allow people to "control" the whole state, then the whole state structure is public API and therefore we should never do breaking change on it between major releases. And that is an important constraints that I think we should not have, especially for a (prop.state) that is not really usable.

The goal here is to limit this external state to a subset of the state on which we will make no breaking change between major releases and to leave all the internals private.

Allow to initialize the filter / pagination, ... without controlling them

The following example talks about the filters but the same thing applies to other controllable states.
On the current version, either you do not set any filter on mount, or you have to fully control the filter. If you only pass a filterModel and no onFilterModelChange, you have a broken UI because you can try to change the values of the input / selects but they will never be applied.
We should probably disable the filter panel when we are in this scenario in another PR.
You have an example of this breaking UI is the 1st example of the filtering doc : https://mui.com/components/data-grid/filtering/#basic-filter

But I think it is a 100% valid use case to want to give an initial filter without controlling it. This is now possible by giving an initial state

<DataGrid 
  {...baselineProps} 
  initialState={{
    filter: {
      filterModel: {
        items: [
          { id: 1, columnField: 'commodity', operatorValue: 'contains', value: 'rice' },
          { id: 2, columnField: 'commodity', operatorValue: 'startsWith', value: 'Soy' },
        ],
        linkOperator: GridLinkOperator.Or,
      }
    }
  }}
/>

Ultimately I propose we have the following behaviors

  • prop.filterModel and prop.onFilterModelChange defined : the filtering UI is editable and the state is controlled
  • prop.filterModel defined but prop.onFilterModelChange not defined : the filtering UI is not editable and the state is controlled
  • prop.initialState.filter.filterModel defined : the filtering UI is editable, the state is not controlled but initialized with the prop value
  • prop.filterModel and prop.initialState.filter.filterModel not defined, the filtering UI is editable and initialized with the default value

The same applies to other controllable models.

Prepare the state export

The value taken by initialState should be compatible with what is exported by the future exportState method.
In both cases we want a subset of the states (we do not want to export the internal rows representation for instance).

The users would then be able to do something like

const DataGridWithLocalStorage = () => {
  const [initialState] = React.useState(() => localStorage.getItem('state'));
  const apiRef = useGridApiRef();

  const saveAllState = () => 
    localStorage.setItem('state', apiRef.current.exportState());

  const saveFilterState = () => 
    localStorage.setItem('state', { filter: apiRef.current.exportState().filter });

  return (
    <div>
      <Button onClick={saveAllState}>Save</Button>
      <Button onClick={saveFilterState}>Save Filter</Button>
      <DataGridPro 
        {...baselineProps} 
        initialState={initialState} 
        apiRef={apiRef} 
      />
    </div>
  )
}

Internal behavior

  • Each feature hook defines an initialState interface (which should probably always be a subset of its state to keep things simple)
  • Each feature hook is responsible for its state initialization (in useGridStateInit)
  • If the feature hook also have a control state, the control state should always win
  • The initial state should be fully optional, no feature hook should require it to be defined or to have a value required if defined

Docs

I have a few questions here about how we want to organize our doc examples @m4theushw @DanailH

  • Do we want to add a section in the filter and sorting pages to tell how to initialize those states. Or do we want a single centralized section in a new page ?

  • Do we want our doc examples to use initialState or the control model when we only want to show a static value (not to show how the control state works). The filter example uses prop.filterModel without prop.onFilterModelChange so they are broken and I migrated them to prop.initialState. But sorting examples always define both prop.sortModel and prop.onSortModelChange so they work. Both make sense, we just have to decide.

We lack good docs examples for the control state (except for the pagination).
I would be in favor of using the initialState in the docs examples except for the controlled one to have a clear distinction between controlled and not controlled.

Breaking change

If you only want to set the initial value of a state (enable for preferencePanel, filter.filterModel and sort.sortModel)

<DataGrid
-  state={{
+  initialState={{
    preferencePanel: {
      open: true,
      openedPanelValue: GridPreferencePanelsValue.filters,
    },
  }}
/>

If you want to fully control the state, use the filterModel / onFilterModelChange, sortModel / onSortModelChange, page / onPageChange, pageSize / onPageSizeChange props.

@flaviendelangle flaviendelangle self-assigned this Oct 12, 2021
@flaviendelangle flaviendelangle changed the title [DataGrid] Replace prop.state with prop.initialState [DataGrid] Replace prop.state with prop.initialState Oct 12, 2021
@flaviendelangle flaviendelangle added breaking change component: data grid This is the name of the generic UI component, not the React module! labels Oct 12, 2021
@flaviendelangle flaviendelangle added this to the v5 stable version milestone Oct 12, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 12, 2021
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 12, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 13, 2021
@mui mui deleted a comment from github-actions bot Oct 13, 2021
@mui mui deleted a comment from github-actions bot Oct 13, 2021
@flaviendelangle flaviendelangle marked this pull request as ready for review October 13, 2021 09:01
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 14, 2021
@mui mui deleted a comment from github-actions bot Oct 15, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 15, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 18, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 18, 2021
@DanailH
Copy link
Member

DanailH commented Oct 18, 2021

@flaviendelangle I check the PR and for me, it makes perfect sense to have this separation and to hide the state.
Am I understand correctly that we will add initialState entry on a per-feature basis - so we can treat it the same as deciding on the public API. What I'm trying to say is that when we work on a feature we need to not only decide on the API methods we would expose but also the entries in the initialState prop.

Regarding the docs - we can have a single page explaining the initialState and on it have a couple of exaples.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Oct 18, 2021

What I'm trying to say is that when we work on a feature we need to not only decide on the API methods we would expose but also the entries in the initialState prop.

Yes
And for me, adding the new feature to the initialState can be done afterward if there are people asking for it.
I don't think there are tons of entries to add. For instance for the Tree Data I see none.

@flaviendelangle
Copy link
Member Author

I will document the initialState in a follow up PR 👍

@flaviendelangle flaviendelangle merged commit 1d56ac5 into mui:next Oct 19, 2021
@flaviendelangle flaviendelangle deleted the initialState branch October 19, 2021 10:31
@flaviendelangle flaviendelangle mentioned this pull request Oct 22, 2021
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.

None yet

2 participants