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

[material-ui] Add storageManager prop to ThemeProvider #45136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jan 29, 2025

closes #35837

  • Implementation and tests
  • Update docs

Changes

  • Added new prop storageManager to the ThemeProvider. The use cases I see:
    • Disable the storage behavior by providing storageManager={null}
    • Change the storage from localStorage to a custom one e.g. cookies.
  • Extract localStorage logic to localStorageManager.
  • Export type StorageManger from @mui/material/styles so user can create a custom one with type safety.

@siriwatknp siriwatknp added package: system Specific to @mui/system package: material-ui Specific to @mui/material labels Jan 29, 2025
@mui-bot
Copy link

mui-bot commented Jan 29, 2025

Netlify deploy preview

@material-ui/system: parsed: +0.39% , gzip: +0.51%
@material-ui/core: parsed: +0.06% , gzip: +0.09%
@mui/joy: parsed: +0.07% , gzip: +0.11%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b7edd82

@oliviertassinari oliviertassinari changed the title [material-ui] Add storageManager prop to ThemeProvider. [material-ui] Add storageManager prop to ThemeProvider Jan 29, 2025
@siriwatknp siriwatknp marked this pull request as ready for review January 31, 2025 07:49
@@ -70,6 +71,11 @@ export interface CreateCssVarsProviderResult<
* @default document
*/
colorSchemeNode?: Element | null;
/**
* The storage manager to be used for storing the mode and color scheme
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The storage manager to be used for storing the mode and color scheme
* The storage manager to be used for storing the mode and color scheme.


## Storage manager

The default approach of the [built-in support](#built-in-support) uses the browser's `localStorage` API to store the mode and color scheme preference.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The default approach of the [built-in support](#built-in-support) uses the browser's `localStorage` API to store the mode and color scheme preference.
By default, the [built-in support](#built-in-support) for color schemes uses the browser's `localStorage` API to store the user's mode and scheme preference.

@@ -342,3 +342,57 @@ For applications that need to support light and dark mode using CSS media `prefe

But if you want to be able to toggle between modes manually, avoiding the flicker requires a combination of CSS variables and the `InitColorSchemeScript` component.
Check out the [Preventing SSR flicker](/material-ui/customization/css-theme-variables/configuration/#preventing-ssr-flickering) section for more details.

## Storage manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the best location for this info? Or would it make sense as an h3 under the System preference header? The fact that it refers back to the Built-in support section for context makes me think they should be located near each other. But if you have a strong opinion about this placement then I'm sure it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be h2 because it's a different topic similar to Toggling color mode and the reason I put this section at the end is because I expect only a few projects looking for this docs.

Do you think it should be moved next to Toggling color mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, in that case yes I think placing it near Toggling color mode makes sense. If you read these two sections back-to-back it does have a logical flow, even if this new addition is somewhat of an edge case.

</ThemeProvider>
);
}
```
Copy link
Member

Choose a reason for hiding this comment

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

We should also include an example of how to turn it off entirely with storageManager={null}, including the consequences of doing so.

Comment on lines 180 to 187
try {
localStorage.setItem(modeStorageKey, newMode);
} catch {
if (modeStorage) {
modeStorage.set(newMode);
}
} catch (error) {
// Unsupported
}
return {
Copy link
Member

Choose a reason for hiding this comment

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

modeStorage already has a try/catch on its set method, so this should be equivalent:

if (modeStorage) {
    modeStorage.set(newModa)
}

Without the redundant try/catch, or am I missing something?

@@ -45,7 +56,7 @@ describe('useCurrentColorScheme', () => {
// clear the localstorage
storage = {};
// Create mocks of localStorage getItem and setItem functions
Object.defineProperty(global, 'localStorage', {
Object.defineProperty(window, 'localStorage', {
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for the global -> window changes?

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

We should cherry-pick this to v6.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add abillity to prevent CssVarsProvider from using local storage
5 participants