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] Pass the stylesheet directly to GlobalStyles #43739

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Sep 13, 2024

To unblock #43708

Added regression test: https://app.argos-ci.com/mui/material-ui/builds/32120/109050219

Root cause

After the fix from #43632, if <StyledEngineProvider injectFirst> is used, the global style injection order is included in the Emotion normal process like styled API. Meaning that Emotion will process the end of the GlobalStyles first. For example,

Screen.Recording.2567-09-13.at.11.49.57.mov

This causes unexpected behavior because the way CSS variables are generated should be ordered by:

<GlobalStyles styles={{ :root }} />
<GlobalStyles styles={{ light }} /> // inject after dark, this is the bug in https://github.com/mui/material-ui/pull/43708#issuecomment-2346368396
<GlobalStyles styles={{ dark }} />

Fix

This PR put the ordered stylesheet into a single GlobalStyles so that the injection order does not matter.

Screen.Recording.2567-09-13.at.11.51.36.mov

@mui-bot
Copy link

mui-bot commented Sep 13, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 054986f

@zannager zannager requested a review from DiegoAndai September 13, 2024 10:05
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

@siriwatknp could we add a test for this?

@siriwatknp
Copy link
Member Author

siriwatknp commented Sep 16, 2024

@siriwatknp could we add a test for this?

Added a regression test (I think for use case like this, it's best to run the test in the real browser env). Also, tested with sandbox build, it fixes the issue.

https://app.argos-ci.com/mui/material-ui/builds/32120/109050219

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 regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants