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.

Sign up for GitHub

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

Jump to bottom

[utils] Port useLocalStorageState hook from Toolpad #41096

Merged
merged 9 commits into from
Feb 21, 2024
Merged

[utils] Port useLocalStorageState hook from Toolpad #41096

merged 9 commits into from
Feb 21, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 14, 2024 •
edited
Loading

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
Janpot
Janpot commented Feb 14, 2024
View reviewed changes
docs/src/modules/components/AppSettingsDrawer.js
@@ -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.

Sign in to view

@mui-bot
Copy link

mui-bot commented Feb 14, 2024 •
edited
Loading

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

@Janpot Janpot added the package: utils Specific to the @mui/utils package label Feb 14, 2024
@Janpot Janpot mentioned this pull request Feb 14, 2024
Open
@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
bharatkashyap
bharatkashyap approved these changes Feb 19, 2024
View reviewed changes
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.

🎉

siriwatknp
siriwatknp reviewed Feb 20, 2024
View reviewed changes
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 •
edited
Loading

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.

siriwatknp
siriwatknp approved these changes Feb 21, 2024
View reviewed changes
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.

@Janpot
Merge branch 'master' into use-local-storage-state
b41efdc 
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@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 •
edited
Loading

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

@Janpot Janpot mentioned this pull request Feb 21, 2024
Merged
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 24, 2024 •
edited
Loading

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
Revert "[utils] Port useLocalStorageState hook from Toolpad (#41096)"
0d086a1 
This reverts commit b923a63.
See for why #41096 (comment)
oliviertassinari pushed a commit to Janpot/material-ui that referenced this pull request Feb 24, 2024
@Janpot @oliviertassinari
[utils] Port useLocalStorageState hook from Toolpad (mui#41096)
95bd5b3 
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
oliviertassinari pushed a commit to Janpot/material-ui that referenced this pull request Feb 25, 2024
@Janpot @oliviertassinari
[utils] Port useLocalStorageState hook from Toolpad (mui#41096)
edc3d33 
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@oliviertassinari @mnajdova
Revert "[utils] Port useLocalStorageState hook from Toolpad (mui#41096
28f40da 
)"

This reverts commit b923a63.
See for why mui#41096 (comment)
This was referenced May 24, 2024
Open
Open
@Crushoverride007 Crushoverride007 mentioned this pull request Jun 5, 2024
Open
This was referenced Jul 16, 2024
Open
Open
Open
@Crushoverride007 Crushoverride007 mentioned this pull request Jul 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siriwatknp siriwatknp siriwatknp approved these changes

@bharatkashyap bharatkashyap bharatkashyap approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation package: utils Specific to the @mui/utils package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Janpot @mui-bot @oliviertassinari @siriwatknp @bharatkashyap