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

[system] Add enableColorScheme option to getInitColorSchemeScript #33261

Merged
merged 1 commit into from Jun 23, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 23, 2022

The enableColorScheme option did not pass to getInitColorSchemeScript. By adding this option, the script attaches the color-scheme before the application is rendered on the screen.

Next step: I will add a script to minimize via terser.


@siriwatknp siriwatknp added the package: system Specific to @mui/system label Jun 23, 2022
@mui-bot
Copy link

mui-bot commented Jun 23, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 81a5f40

@siriwatknp siriwatknp requested a review from mnajdova June 23, 2022 07:58
@@ -75,6 +84,9 @@ export default function getInitColorSchemeScript(options?: GetInitColorSchemeScr
if (colorScheme) {
${colorSchemeNode}.setAttribute('${attribute}', colorScheme);
}
if (${enableColorScheme} && !!cssColorScheme) {
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
if (${enableColorScheme} && !!cssColorScheme) {
if (enableColorScheme && !!cssColorScheme) {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

if (${enableColorScheme} && !!cssColorScheme) { is correct because the whole function is in a literal template string.

Copy link
Member

Choose a reason for hiding this comment

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

Ah it is a variable outside of the __html string, makes sense 👍

@siriwatknp siriwatknp merged commit 9a318c9 into mui:master Jun 23, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 26, 2022

@siriwatknp This change breaks the dark mode on http://mui.com/, it was released in v5.8.6. Compare:

Screenshot 2022-08-27 at 01 12 37

Screenshot 2022-08-27 at 01 12 33

In practice, it's a regression from #29454. I think that the issue is here:

Screenshot 2022-08-27 at 01 13 58

Shouldn't we get a single light/dark mode for @mui/system, and have Material UI/Joy UI directly rely on it?

We might also want to remove <html style="color-scheme: light;"> It's a massive global effect, something that feels like should be the responsibility of CssBaseline: https://mui.com/material-ui/react-css-baseline/#color-scheme. So 👍 to drop enableColorScheme from createCssVarsProvider.

maybe something like this:

diff --git a/packages/mui-material/src/CssBaseline/CssBaseline.js b/packages/mui-material/src/CssBaseline/CssBaseline.js
index 2b32bd5789..39579fab5a 100644
--- a/packages/mui-material/src/CssBaseline/CssBaseline.js
+++ b/packages/mui-material/src/CssBaseline/CssBaseline.js
@@ -11,7 +11,11 @@ export const html = (theme, enableColorScheme) => ({
   boxSizing: 'border-box',
   // Fix font resize problem in iOS
   WebkitTextSizeAdjust: '100%',
-  ...(enableColorScheme && { colorScheme: theme.palette.mode }),
+  ...(enableColorScheme && {
+    [theme.getColorSchemeSelector('dark')]: {
+      colorScheme: 'dark',
+    },
+  }),
 });

 export const body = (theme) => ({

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 4, 2022

@siriwatknp Could you prioritize fixing this regression? It seems important, thanks

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

Successfully merging this pull request may close these issues.

None yet

4 participants