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

[utils] Port useLocalStorageState hook from Toolpad #41096

Merged
merged 9 commits into from
Feb 21, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 14, 2024

As per mui/mui-x#11576 (comment)

  • Port useLocalStorageState hook from Toolpad.
  • Replace theme switcher implementation with this hook
  • Replace package manager switcher with this hook.

Immediate improvements this hook brings to the docs

@Janpot Janpot added the docs Improvements or additions to the documentation label Feb 14, 2024
@@ -42,44 +43,25 @@ function AppSettingsDrawer(props) {
const t = useTranslate();
const upperTheme = useTheme();
const changeTheme = useChangeTheme();
const [mode, setMode] = React.useState(null);
const [mode, setMode] = useLocalStorageState('mui-mode', 'system');

This comment was marked as off-topic.

@mui-bot
Copy link

mui-bot commented Feb 14, 2024

Netlify deploy preview

https://deploy-preview-41096--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b41efdc

@oliviertassinari
Copy link
Member

Changes like this one create an extra incentive to solve #35840.

@Janpot Janpot marked this pull request as ready for review February 15, 2024 09:55
@Janpot Janpot requested a review from a team February 15, 2024 09:55
@flaviendelangle flaviendelangle changed the title [utils] Port useLocalStorageState hook from Toolpad [utils] Port useLocalStorageState hook from Toolpad Feb 15, 2024
Copy link
Member

@bharatkashyap bharatkashyap left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Does useLocalStorageState need to be exported from @mui/utils?

@Janpot
Copy link
Member Author

Janpot commented Feb 20, 2024

I was planning to replace our usage in toolpad with this implementation. X was also investigating whether they could provide datagrid persistence on top of it.
I'm happy to move it wherever though, this seemed like a logical spot to me.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 20, 2024
@oliviertassinari
Copy link
Member

X was also investigating whether they could provide datagrid persistence on top of it

What if it was Toolpad's scope to deliver this? Maybe that module could also have an adapter for TanStack Table, for AG Grid.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 I think let's reexport from @mui/utils too for consistency.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2024
@Janpot Janpot merged commit b923a63 into mui:master Feb 21, 2024
22 checks passed
@Janpot Janpot deleted the use-local-storage-state branch February 21, 2024 09:13
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 21, 2024

This introduces regressions:

  1. The homepage dark mode toggle doesn't work anymore:
SCR-20240221-ndea

https://deploy-preview-41096--material-ui.netlify.app/

  1. The try/catch to support private mode was removed. To add back, e.g. [system] Catch localStorage unavailability #34027

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 24, 2024

3rd regressions: which I can reproduce on https://deploy-preview-41096--material-ui.netlify.app/material-ui/react-button/ and in localhost:

RDvmpjl0rr2F38jr.mp4

For example: https://twitter.com/apoorv_taneja/status/1761292886052884955.

I have reverted with 0d086a1.
I have done a follow-up on this, cleaning the logic #41223, which fixed the problem. I don't know exactly how, once I removed the redundant rerenders, the loop stopped but the dark/light mode was clearly broken. This was because Material UI and Material UI Next share the same dark/light locale storage value but also we were trying to sync Material UI Next light/dark mode with Material UI, which doesn't make sense if the locale storage value and DOM attributes are the same. I removed that and it works now.

oliviertassinari added a commit that referenced this pull request Feb 24, 2024
oliviertassinari pushed a commit to Janpot/material-ui that referenced this pull request Feb 24, 2024
oliviertassinari pushed a commit to Janpot/material-ui that referenced this pull request Feb 25, 2024
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: utils Specific to the @mui/utils package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants