mui-x
mui-x copied to clipboard
mui
[data grid] Improve "Save and restore the state from external storage" demo
Summary
This is a continuation of #7876. The demo https://next.mui.com/x/react-data-grid/state/#save-and-restore-the-state-from-external-storage is a good starting point but I don't feel that is good enough yet. I see 3 opportunities:
- There is a layout shift when loading. I think that it would be best to remove the CircularProgress and to reserve the needed vertical height.
- There is no state reset button, to go back to the default data grid state
- And last but the most important, the state doesn't save when unmounting the data grid:
https://github.com/mui/mui-x/assets/3165635/dd1353cd-c514-449e-b35d-b16270d0a25c
Examples
No response
Motivation
No response
Search keywords: Save and restore the state from external storage
For inspiration: In Toolpad we wrote a hook that handles initializing state from localstorage on the first render correctly (i.e. doesn't need an effect to run). It also syncs this state across tabs.
@Janpot What do you think about moving the useStorageStateBrowser hook to @mui/utils?
I think we have many utility hooks we use internally that our users could use.
We can create a separate package and docs for these hooks - e.g. @mui/hooks.
This could also be an initiative under the MUI X product line - @mui/x-hooks with more advanced utility hooks like useStorageStateBrowser, useResizeObserver, useTimeout, useFirstRender, useOnMount, etc.
cc @joserodolfofreitas @oliviertassinari @mui/x
To be noted that @mui/utils is a "private" npm package. Only MUI System and Base UI are supposed to import from it.
This package exists so that MUI System doesn't have a dependency on Base UI (would be weird, and isolate download npm stats) but still able to use some of its logic to create components.
To be noted that @mui/utils is a "private" npm package. Only MUI System and Base UI are supposed to import from it. This package exists so that MUI System doesn't have a dependency on Base UI but still able to use some of its logic to create components.
Just to note that this example is promoting @mui/utils as a public package. (This is why we have it as a dependency in Toolpad, I never knew it wasn't meant to be public)
Other places where @mui/utils seems to be endorsed in the docs as a public package:
visuallyHiddenhttps://mui.com/material-ui/icons/#semantic-iconsvisuallyHiddenhttps://mui.com/material-ui/migration/v5-component-changes/#remove-sronly-variantunstable_useForkRefhttps://mui.com/base-ui/react-autocomplete/#using-a-portal (promoting anunstable_export on an internal package 🙂 )
IMO, the @mui/utils package is already de facto public. It would make more sense to mark everything in the package as unstable_ to signal to consumers that it's not following semver guarantees. It's more effective than documenting it in the docs or README, at least you notice it when vscode autoimports something from the @mui/utils package.
To be noted that @mui/utils is a "private" npm package. Only MUI System and Base UI are supposed to import from it. This package exists so that MUI System doesn't have a dependency on Base UI but still able to use some of its logic to create components.
Just to note that this example is promoting
@mui/utilsas a public package. (This is why we have it as a dependency in Toolpad, I never knew it wasn't meant to be public)Other places where
@mui/utilsseems to be endorsed in the docs as a public package:
visuallyHiddenhttps://mui.com/material-ui/icons/#semantic-iconsvisuallyHiddenhttps://mui.com/material-ui/migration/v5-component-changes/#remove-sronly-variantunstable_useForkRefhttps://mui.com/base-ui/react-autocomplete/#using-a-portal (promoting anunstable_export on an internal package 🙂 )IMO, the
@mui/utilspackage is already de facto public. It would make more sense to mark everything in the package asunstable_to signal to consumers that it's not following semver guarantees. It's more effective than documenting it in the docs or README, at least you notice it when vscode autoimports something from the@mui/utilspackage.
What would prevent us from following semver? Couldn't we make this public and follow best practices as well?
What would prevent us from following semver?
Nothing prevents us, it just adds overhead. It needs to be properly maintained, backwards compatibility needs to be guaranteed more tightly, and it needs to be documented.
edit: To be clear, I interpreted the question as "what prevents us from treating @mui/utils as an officially supported MUI product?".
I don't think making it public is a good idea. The code in there is internal code that we need to share across our packages/repos, not code we want the public to consume. I think we should either clearly mark it as a private package or create a new one with a name like @mui/internals. Then there is no issue with removing the unstable_ prefix and making breaking changes to the code, because it's an internal-only package. There is also nothing that prevents us from uncoupling the version number from the core's versionning scheme if we introduce breaking changes.
What would prevent us from following semver?
Nothing prevents us, it just adds overhead. It needs to be properly maintained, backwards compatibility needs to be guaranteed more tightly, and it needs to be documented.
I don't think semver implies we need to do all that. If it's an internal package we can have a meaningful (semver) version number while also not documenting it. Following semver doesn't imply we make the package public. We could also just leave it at version 0.x, which semver defines as basically "do anything you want" (point 4 of semver).
Other places where @mui/utils seems to be endorsed in the docs as a public package:
@Janpot Yes, it's not great, this work was never prioritized.
The shaping page that centralizes the discussions on this topic: https://www.notion.so/mui-org/mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95.
I think we should either clearly mark it as a private package or create a new one with a name like @mui/internals.
@romgrk this makes sense to me. Essentially have 2 packages,@mui/internals which is mostly what is currently in the utils, and @mui/utils which is public and contains things like useStorageStateBrowser from Toolpad. I can see us adding more relevant utils for our users there, we are probably going to encounter some cases when we ship the data grid server-side integration (transformers, adaptors, etc.)
I ported the hook to @mui/utils. For now with the purpose to only use it internally. If anyone has objections to this, please comment on the PR.
To build save/store grid state on top of this, it will need to be wrapped with parsing/serialization/validation logic. The hook doesn't mean to be opinionated about parsing, error handling and migrating stored values.
So, I did try to fix this, but apparently the mechanism for storing works, but the initialState does not get applied correctly
In the video below it can be observed, that the state I get from localStorage is showing a different column dimension and order than the grid renders with. Column pinning works fine for some reason:
https://github.com/mui/mui-x/assets/32863416/257b8161-f2f8-424b-b47c-7b30b1da54a3
I have literally no idea why the state does not get applied in the docs ... I tried to with a lot of different approaches, but it won't work.
It does work perfectly in the playground and in stackblitz 🤯
Weird. Maybe the vibrations aren't harmonious enough in the docs. But it also feels like it could be a datagrid bug, not necessarily a docs issue.
+1 on this: we noticed this issue turned a bit stale/inactive, but it is still affecting our application that is very DataGrid heavy. Our users particularly want the column visibility, order, and widths to persist on page refresh/navigation from their own preference and choice, which the Save and Restore state is exactly what we need for this. See demo over on: #13963 Thank you
@michelengelen
I have literally no idea why the state does not get applied in the docs ... I tried to with a lot of different approaches, but it won't work.
It does work perfectly in the playground and in stackblitz 🤯
I think the reason is point (3) from the issue description.
The demo reset button unmounts the Data Grid and mounts it again. Apparently, the state is not saved in that case.
The page refresh fires the beforeunload event, which in turn stores the state.
