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.

Sign up for GitHub

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

Jump to bottom

[docs-infra] Fix dark/light mode #39077

Open
wants to merge 3 commits into
base: v5.x
Choose a base branch
from
Open

[docs-infra] Fix dark/light mode #39077

wants to merge 3 commits into from

Conversation

ttlpta
Copy link
Contributor

@ttlpta ttlpta commented Sep 20, 2023 •
edited by oliviertassinari
Loading

@mui-bot
Copy link

mui-bot commented Sep 20, 2023 •
edited
Loading

Netlify deploy preview

https://deploy-preview-39077--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against c3d8bb2

@zannager zannager added the scope: docs-infra Specific to the docs-infra product label Sep 20, 2023
@oliviertassinari oliviertassinari mentioned this pull request Sep 20, 2023
Closed
1 task
@zannager zannager requested a review from siriwatknp September 20, 2023 16:02
oliviertassinari
oliviertassinari reviewed Sep 20, 2023
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

What I can notice

MarkdownDocsV2 should be handled too.

docs/src/modules/components/MarkdownDocs.js Outdated
@@ -46,10 +54,22 @@ export default function MarkdownDocs(props) {
const localizedDoc = docs[userLanguage] || docs.en;

const isJoy = canonicalAs.startsWith('/joy-ui/') && !disableCssVarsProvider;
Copy link
Member

@oliviertassinari oliviertassinari Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should rely on PageContext productId to know the docs environment we are into. It's likely more idiomatic.

   pageContext.productId === 'joy-ui'

docs/src/modules/components/MarkdownDocs.js Outdated
@@ -46,10 +54,22 @@ export default function MarkdownDocs(props) {
const localizedDoc = docs[userLanguage] || docs.en;

const isJoy = canonicalAs.startsWith('/joy-ui/') && !disableCssVarsProvider;
const CssVarsProvider = isJoy ? JoyCssVarsProvider : React.Fragment;
const Wrapper = isJoy ? BrandingProvider : React.Fragment;
const isMui = canonicalAs.startsWith('/material-ui/') && !disableCssVarsProvider;
Copy link
Member

Choose a reason for hiding this comment

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

Not the same

Suggested change
const isMui = canonicalAs.startsWith('/material-ui/') && !disableCssVarsProvider;
const isMaterialUi = canonicalAs.startsWith('/material-ui/') && !disableCssVarsProvider;

docs/src/modules/components/MarkdownDocs.js Outdated
@@ -12,16 +19,17 @@ import BrandingProvider from 'docs/src/BrandingProvider';
import Ad from 'docs/src/modules/components/Ad';
import AdGuest from 'docs/src/modules/components/AdGuest';

function JoyModeObserver({ mode }) {
function ModeObserver({ mode, useColorScheme }) {
const { setMode } = useColorScheme();
React.useEffect(() => {
Copy link
Member

@oliviertassinari oliviertassinari Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A layout effect would make more sense. See the current bug:

Suggested change
React.useEffect(() => {
useEnhancedEffect(() => {
Screen.Recording.2023-09-20.at.18.05.57.mov

But the pattern used feels wrong in the first place. I would expect that when we change the mode, to change the mode instantly, so useColorScheme shouldn't be called here in an effect. Instead, it should be called where we set the dark/light mode. Could we try that instead?

Copy link
Contributor Author

@ttlpta ttlpta Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @oliviertassinari ,

After trying to fix following your comment, I found bugs related to using Experimental_CssVarsProvider of mui-material/styles. the reason is object defaultTheme of material-ui is experimental.

So I think in the scope of this bug #38575, we should only update the attribute "data-mui-color-scheme" by using js function setAttribute. And move optimizing material-ui defaultTheme to another issue which has label customization: theme

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 20, 2023
@ttlpta ttlpta force-pushed the fix/dark-light-mode branch from f7913d9 to cbc7ff3 Compare September 21, 2023 04:27
@oliviertassinari oliviertassinari removed their request for review September 21, 2023 08:46
oliviertassinari
oliviertassinari requested changes Sep 21, 2023
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

We need to fix the root

@ttlpta ttlpta force-pushed the fix/dark-light-mode branch from 72965fa to c3d8bb2 Compare September 22, 2023 01:15
@siriwatknp
Copy link
Member

The docs theming pattern needs to be restructured. I will need to create a new issue with some proposal to move forward.

@mj12albert mj12albert mentioned this pull request Oct 3, 2023
Closed
2 tasks
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 21, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari requested changes

@siriwatknp siriwatknp Awaiting requested review from siriwatknp

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work PR: out-of-date The pull request has merge conflicts and can't be merged scope: docs-infra Specific to the docs-infra product
Projects
Docs-infra
Status: Stalled
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[website] Dark mode toggle doesn't affect [data-mui-color-scheme]
6 participants
@ttlpta @mui-bot @siriwatknp @oliviertassinari @zannager @davidphamntq