-
Notifications
You must be signed in to change notification settings - Fork 357
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
fix: Dark Mode style regressions #11123
base: develop
Are you sure you want to change the base?
fix: Dark Mode style regressions #11123
Conversation
@@ -25,6 +25,7 @@ | |||
"@lukemorales/query-key-factory": "^1.3.4", | |||
"@mui/icons-material": "^5.14.7", | |||
"@mui/material": "^5.14.7", | |||
"@mui/utils": "^5.14.7", |
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 does add a new explicit dependency but I don't think it's doing much harm. It is already installed by yarn because @mui/material
depends on it
If anyone prefers we use a custom deepMerge
like we did previously, happy to discuss
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 that's great cause we already importing this in many places, so another step closer to PNPM
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.
Oh wow. Good call. I didn't even realize that!
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.
Confirming regressions gone, ty!
@@ -25,6 +25,7 @@ | |||
"@lukemorales/query-key-factory": "^1.3.4", | |||
"@mui/icons-material": "^5.14.7", | |||
"@mui/material": "^5.14.7", | |||
"@mui/utils": "^5.14.7", |
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 that's great cause we already importing this in many places, so another step closer to PNPM
@@ -107,4 +108,4 @@ declare module '@mui/material/styles/createTheme' { | |||
|
|||
export const inputMaxWidth = _inputMaxWidth; | |||
export const light = createTheme(lightTheme); | |||
export const dark = createTheme(lightTheme, darkTheme); | |||
export const dark = createTheme(deepmerge(lightTheme, darkTheme)); |
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.
👍
Description 📝
@mui/system
imports #11081createTheme
Related Docs 📖
See https://v5.mui.com/material-ui/customization/theming/#createtheme-options-args-theme
I think the key point is
Calling
createTheme
before merging the raw options has implications that I didn't foresee. CallingcreateTheme
with the theme options merged first fixes the issues we are seeingScreenshots of the Issues 📷
How to test 🧪
As an Author I have considered 🤔