-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add ability to default to dark mode when users prefer it #8874
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved and rewrote a lot of things in there. Best to read the new version rather than read the diff.
return ( | ||
<CoreAdminContext {...rest}> | ||
<ThemeProvider theme={theme}>{children}</ThemeProvider> | ||
<ThemesContext.Provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond of this syntax (ThemesContextProvider wraps ThemeProvider), but it's a good way to make the whole change backwards compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a fixme in v5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the existing FIXMEs introduced in this PR will suffice
@@ -85,10 +85,10 @@ const theme = createTheme({ | |||
|
|||
declare module '@mui/material/styles' { | |||
interface Palette { | |||
userDefined: PaletteColor; | |||
userDefined?: PaletteColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was making TS compilation fail in other tests
Switching to RFR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
return ( | ||
<CoreAdminContext {...rest}> | ||
<ThemeProvider theme={theme}>{children}</ThemeProvider> | ||
<ThemesContext.Provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a fixme in v5?
export interface ThemesContextValue { | ||
darkTheme?: RaThemeOptions; | ||
lightTheme?: RaThemeOptions; | ||
defaultToLightTheme?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no a defaultTheme
instead, allowing you to default to the dark theme if you want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 You're right, it's better. Added in the next commit.
Co-authored-by: Gildas Garcia <[email protected]>
Problems
Solution
<Admin darkTheme>
prop, and a user prefers dark mode, the app defaults to dark modeuseTheme
hook now saves a theme mode ('light' or 'dark') in the preferences rather than a theme object<ToggleThemeButton>
no longer needs theme propsThe solution is made backwards compatible by keeping the old
<ToggleThemeButton>
.Example
Closes #8670
<Admin darkTheme>
prop<Admin defaultToLighTheme>
propThemesContext
<ToggleThemeButton>
to use it<ToggleThemeButton>
by default when a dark mode is defined<ToggleThemeButton>
in one of our demos (ecommerce)