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

[docs] Hotfix missing styles in dark mode #35179

Merged
merged 1 commit into from Nov 17, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Nov 17, 2022

Preview: https://deploy-preview-35179--material-ui.netlify.app/material-ui/api/alert/

Reported in #34976 (comment).

@siriwatknp I have found a regression from this PR (looking a bit at https://mui-org.slack.com/archives/C042VP80PT5/p1667372692296329):

The commit on master just before this PR: https://636d3dbaea7a9900083c33e1--material-ui.netlify.app/material-ui/api/alert/

Screenshot 2022-11-16 at 23 33 37

This PR: https://deploy-preview-34976--material-ui.netlify.app/material-ui/api/alert/

Screenshot 2022-11-16 at 23 33 43

After: Screen Shot 2565-11-17 at 12 47 23

My mistake. Forgot the case where the tokens have different values between color modes.

will do a hotfix once the CIs are green.


@siriwatknp siriwatknp added docs Improvements or additions to the documentation regression A bug, but worse labels Nov 17, 2022
@mui-bot
Copy link

mui-bot commented Nov 17, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35179--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against d920d2a

@siriwatknp siriwatknp merged commit 4cf3c84 into mui:master Nov 17, 2022
the-mgi pushed a commit to the-mgi/material-ui that referenced this pull request Nov 17, 2022
@cherniavskii
Copy link
Member

Noticed this in MUI X docs as well: https://deploy-preview-6773--material-ui-x.netlify.app/x/api/data-grid/data-grid/
Thanks for the fix, will upgrade the monorepo on our side! 🎉

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Nov 19, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 20, 2022

@siriwatknp To confirm if I understand this correctly: Once we have the CssVarsProvider on all the pages, we can remove the whole ':where(.mode-dark) &': { block?

@siriwatknp
Copy link
Member Author

@siriwatknp To confirm if I understand this correctly: Once we have the CssVarsProvider on all the pages, we can remove the whole ':where(.mode-dark) &': { block?

Not the whole block (otherwise the dark mode does not work), we could replace the .mode-dark with the data attribute from the theme.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 21, 2022

@siriwatknp Right, my bad. So more like this?

Once we have the CssVarsProvider provider on all the pages, we can then remove all the CSS properties in ':where(.mode-dark) &': { that rely on a var(--muidocs-xxx). So effectively we will be able to remove 80% of the code inside this block.

It sounds good :).

Ideally, I think that it would be great to be able to replace the var(--muidocs-xxx) with theme.vars.xxx values for type safety. I imagine that it would allow us to catch future regressions.

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants