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] Add methods to save and restore the state #820

Closed
1 task done
dheeraj1447 opened this issue Jan 8, 2021 · 17 comments · Fixed by #3593 or #4028
Closed
1 task done

[DataGrid] Add methods to save and restore the state #820

dheeraj1447 opened this issue Jan 8, 2021 · 17 comments · Fixed by #3593 or #4028
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request priority: important This change can make a difference

Comments

@dheeraj1447
Copy link

dheeraj1447 commented Jan 8, 2021

Currently, no column re-order events are exposed on the tables both (DataGrid & XGrid). Need this feature as we can handle custom events while the column reordering happens.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

When column A is dragged to column B. I should be able to track it.

Examples 🌈

  • There is already a ColumnReorderApi present as part of the table but need to expose those as event handlers.

Motivation 🔦

  • This will help the users to track column as when the page reloads we still show the same view without reordering the columns to the initial state.

Benchmark

@dheeraj1447 dheeraj1447 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 8, 2021
@DanailH
Copy link
Member

DanailH commented Jan 8, 2021

Hi @dheeraj1447, quick question. Did you try doing apiRef.current.subscribeEvent(COL_REORDER_START, yourHandlerHere); ?

There is a list of events that are being fired when columns are reordered, you can check them HERE

@dheeraj1447
Copy link
Author

Yes @DanailH I tried that.. It would be lot better If that can be exposed on the table itself. Because ApiRef can only be accessible through functional components, If I'm not wrong.

@DanailH
Copy link
Member

DanailH commented Jan 8, 2021

Yes @dheeraj1447 ApiRef comes out of useApiRef which is a hook.

Maybe I'm not understanding the problem correctly, can you help me understand what do you call a table, is it the rows of the grid?

Or maybe you are reordering columns using the API, in which case these events won't be fired?

@dheeraj1447
Copy link
Author

dheeraj1447 commented Jan 8, 2021

Since ApiRef can only be used in a functional components, It would be better If the column re-order events are exposed on XGrid or DataGrid's declarative API. Take the custom event handler as a prop on these just like how you take event handlers onPageChange. OnSelectionChange etc.

For example,

<XGrid {...props} onColReorderStart={customHandler} />

@DanailH hope that gives you a clear picture on what I'm suggesting for. Thanks!

@oliviertassinari oliviertassinari changed the title [DataGrid] expose column reorder events on the table [DataGrid] Allow saving the state of the grid in a database for resuming previous session Jan 8, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Allow saving the state of the grid in a database for resuming previous session [DataGrid] Allow saving the state in DB for resuming previous session Jan 8, 2021
@oliviertassinari oliviertassinari added new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 8, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 8, 2021

This will help the users to track column as when the page reloads we still show the same view without reordering the columns to the initial state.

@dheeraj1447 I don't think that we need to solve the reorder event problem in isolation. What we need to solve is how to store the state of the grid between two sessions. Handling each state manually won't scale well (order of the columns, size of the column, sorting applied, density applied, etc). Is this correct?

We have started to work on it in #533, however, we aren't here yet. I'm at least aware of the problem problems to solve:

  1. What's inside the public state:
  • Remove derivative value from the public state. For instance, it doesn't make sense to expose state.columns.lookup to the public state, this is a cache.
  • Remove state.options from the public state. These values are provided by the developers to the React component. They are systematically overridden, they are never a source of truth.
  • Remove state.scrollBar from the public state. These values are computed to match the specification of the user agent.
  1. What's documented. While there is now a onStateChange prop, there are no documentations on how to use it. So it's still a private API.
  2. We miss a defaultState prop, while the state prop is meant for the controlled use case, it seems that @dheeraj1447 can get away with only setting the initial state with an uncontrolled behavior.
  3. More generally speaking resolve [RFC] Change API state #190.

<XGrid {...props} onColReorderStart={customHandler} />

Once you have this callback, how do you intent to feed the data back into the state in the next session?

I think that the API could look like:

<DataGrid
  defaultState={defaultState}
  onStateChange={(state) => {
    saveState({
      filterModel: state.filterModel,
      sortingModel: state.sortingModel,
      hiddenColumn: state.hiddenColumn,
      // Ignore everything else
    })
  }}
/>

@knave23
Copy link

knave23 commented Mar 22, 2021

Perhaps this is a dumb idea, but my expectation was that I could use a useState variable to hold my columns, and that any change within the X-Grid would get reflected back out to my variable and I could listen to changes on it. I'm pretty sure I've seen and used this behavior on other components like Autocomplete and I've found it very useful.

e.g.

const [columns, setColumns] = useState(getDefaultState());

useEffect(() => {
setDefaultState(columns);
}, [columns]);

return (
<XGrid
...
columns={columns}
/>

@joziahg
Copy link

joziahg commented Jun 14, 2021

Has more work been done to make the state portable? Additionally, it would be cool if we could subscribe to a "central state" object but only certain properties. I'd like to store filters, sorting, and column order, but not selection. onStateChange fires for everything even internal, private caches, & derivated values from the props. It's not a portable state.

I'll probably end up subscribing to the events I want then patching it all together with something like https://material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-state--partial-control-use-state

@m4theushw
Copy link
Member

m4theushw commented Aug 12, 2021

Prerequisite

Besides the problems raised in 1 from #820 (comment), I think we have a couple more keys in the state that need to be reworked to make the state portable:

  • Remove state.columnReorder and state.columnResize and replace with internal state. These keys are used store the column being dragged or resized. We don't need to restore the column that was being dragged before the state was saved.
  • Remove state.containerSizes. The window size could have changed when the session is restored. It could be replaced with a function the computes all the sizes.
  • Remove state.error and use internal state.
  • Remove state.isScrolling. Use events to detect when the grid is scrolling. [core] Remove state.isScrolling #2337
  • Remove state.rendering. The same reason of the container sizes. However, we should save the scroll position.
  • Remove state.rows since it can be obtained from the prop.
  • Remove state.viewport and use a function to obtain the viewport sizes.
  • Don't save the entire column definition in state.columns.lookup. We only need the computed width to restore the sizes of the columns.

And repeating the items from #820 (comment):

  • Remove state.options.
  • Remove state.scrollBar.

@flaviendelangle
Copy link
Member

flaviendelangle commented Aug 26, 2021

We currently have a prop state which is set as the internal state when defined.
What is the use case for it ?

If it's to allow saving / restoring the state in a DB / local storage, then I think it should be renamed initialState and ignore after the first update to avoid strange behaviors.


For the work for portability

Starting by removing the keys that are only used locally like state.isScrolling seems fine to me.

I'm not sure removing the elements like columnReorder / columnResize from the main state is a good idea.
At least it should not make it complicated to pass the data to components that are using it.
Because if the only concern is portability, providing a apiRef.current.exportState which picks only certain keys could also be an option.

More broadly, we need to rework the state coming months, this is the goal of #2231
My end goal is to go back to a true React state (useState not useRef with forceUpdate) to avoid full rerenders and allow React.memo.

I think at this point, any major state change should first have some proof of concept to discuss the tradeoffs done.
Because we won't want to rework it every 4-6 months like we are currently doing.

@oliviertassinari
Copy link
Member

We currently have a prop state which is set as the internal state when defined. What is the use case for it?

From what I recall, Damien added the state prop so that developers could update sub-parts of it, whenever they liked. It's implemented in useStateProp. Allowing developed to restore the state from the DB was part of the use case for it. It was meant to avoid having to control each sub-state. I'm not aware of use case for being able to mutate the state prop after the first render and get the update in the data grid. Renaming it initialState and limiting its behavior sounds great. At least, until we see developers come up with a real use case.

Because if the only concern is portability, providing a apiRef.current.exportState which picks only certain keys could also be an option.

It's one option, I think that we mentioned it in the past. I personally feel that we could already try to add a demo for this problem in the documentation, something developers can use in production. We could have:

  • A onStateChange callback, cherry-picking and storing sub-parts of the state. We would only expose the parts of the state we are confident to make public at this point.
  • An initialState prop, to restore sub-parts of the state, the ones that make sense for restoring the data grid between two sessions.

Then, progressively rework the state for covering more cases, and making the restore DB demo more useful.

@m4theushw
Copy link
Member

m4theushw commented Nov 8, 2021

I think we can pause the effort on cleaning the state #820 (comment) and proceed with adding API methods to allow to save and restore parts of the state. Looking at what is stored and what users seems to ask most, I think we should expose the following items:

interface GridInitialState { // Rename/alias to GridSavedState?
  pagination: GridPaginationState;
  columns: GridColumnsInitialState;
  selection: GridSelectionModel;
  sorting?: GridSortingInitialState;
  filter?: GridFilterInitialState;
  preferencePanel?: GridPreferencePanelInitialState;
  density: GridDensityInitialState;
}

interface GridColumnsInitialState {
  sizes: { [field: string]: number; }; // The user can choose to restore only sizes or column order
  order: string[]
}

interface GridDensityInitialState {
  value: GridDensity; // "standard" | "confortable"
}

The columns key is an array of only the visible columns. If a column is not in this array, it's hidden. The order of the items matches the order of the columns.

As for the API methods, it could be:

apiRef.current.exportState(): GridInitialState
apiRef.current.restoreState(state: GridInitialState): void

I don't know if we should support restoring the session in the initialState. This prop is a 1-to-1 mapping between the state and the value passed to it. On some keys, we need to reconcile the values with the equivalent prop. For instance, from the columns key only the width, order and visibility can be derived, the user must still pass the remaining column definition. Another example is density, we only save the density value but from it derive the row and header height. The prop seems to be useful only to hardcode the state. Looking at its definition, the value restored would never take effect if the same value is being controlled by a prop.

https://github.com/mui-org/material-ui-x/blob/95dd5d648fbeb05f3ce6fe9bd3464c451fe4a755/packages/grid/x-data-grid/src/DataGrid.tsx#L233

@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 9, 2021

This prop is a 1-to-1 mapping between the state and the value passed to it. On some keys, we need to reconcile the values with the equivalent prop

It does not have to be.
The way I built it, the initialState is just an object from which any feature hook can take elements and do whatever it wants with it.
We could totally do the following:

interface GridInitialState {
  ...
  columns: {
    dimensions: { field: string, computedWidth: number }[]
  }
}

The format above is an example, it could be any other format for useGridColumns to process.
I just respected the top level keys of the state, to have each hook handling its initialization while being future proof (if we want to add something else than filterModel handled by useGridFilters for instance).


I built the initialState with this export feature in mind and their should be no conceptual problem to make the two work together.
My point is not necessarily that everything needs to work out of the box for the initialState on day one.
More that we should have the same format between the two.
I see several benefits doing this:

  • Not another format to learn for the user or for us. If they know how to set the filterModel for the initialState, they know how to play with the format returned by exportSession.

  • If we want to be able to inject the value returned by restoreSession in initialState, it could be done without any breaking change.


I agree that the main concern right now is the control models.
I decided to set the priority to the control prop over the initialState. This is a convention, it could be the opposite if we wanted.
If their is a callback defined, then it would call it to set the value from the initialState.
If their is none, we are blocked. But we are also blocked with restoreSession which should not be able to update the state with a model that is being controlled and that has no update method.

@m4theushw
Copy link
Member

I just respected the top level keys of the state, to have each hook handling its initialization while being future proof (if we want to add something else than filterModel handled by useGridFilters for instance).

We can keep the top level keys. I updated #820 (comment) to add them.

I built the initialState with this export feature in mind and their should be no conceptual problem to make the two work together.

Probably restoreState will be on its own hook and there will be all the logic to reconcile each key. If we are going to support initialState, the same logic should be reused.


For the controllable model, if there's no callback we don't have to do anything. The user must add the onXXXChange if he's already controlling a prop and wants to restore the state.

@flaviendelangle flaviendelangle self-assigned this Jan 19, 2022
@flaviendelangle flaviendelangle changed the title [DataGrid] Allow saving the state in DB for resuming previous session [DataGrid] Add methods to save and restore the state Jan 20, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 24, 2022

I'm reopening, the docs still link this issue https://mui.com/components/data-grid/state/#save-and-restore-the-state

Screenshot 2022-03-24 at 12 47 41

From what I understand in #3593, we did an iteration with private APIs: #3593 (review)

@flaviendelangle
Copy link
Member

Closed by #4028

@tielushko
Copy link

The documentation seems to still not be available? https://mui.com/components/data-grid/state/#save-and-restore-the-state

I also happened to notice there was a release of 5.8 on npm, but no release notes on GitHub.

@flaviendelangle
Copy link
Member

The doc of v5.8 will come soon, we have a few problems releasing it.

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 priority: important This change can make a difference
Projects
None yet
8 participants