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

[example] update EmotionCacheProvider to work with GlobalStyles #37962

Merged
merged 4 commits into from
Jul 18, 2023
Merged

[example] update EmotionCacheProvider to work with GlobalStyles #37962

merged 4 commits into from
Jul 18, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 14, 2023 •
edited

closes #37935

Root cause

The previous version include global styles into one style tag:

<style data-emotion="…."> all styles including global</style>

Then when changing to dark mode, the client-side global is inserted but emotion adds it at the top so it be:

<style data-emotion="css-global">client-side global</style> /* can't win the server-side global */
<style data-emotion="…."> all styles including global</style>

The client-side global cannot override the server-side global due to cascading order.

@siriwatknp siriwatknp added the examples Relating to /examples label Jul 14, 2023
@siriwatknp siriwatknp requested a review from a team July 14, 2023 11:01
@mui-bot
Copy link

mui-bot commented Jul 14, 2023 •
edited

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against c574228

@siriwatknp siriwatknp removed the request for review from a team July 14, 2023 17:20
@siriwatknp siriwatknp marked this pull request as draft July 14, 2023 17:20
@siriwatknp siriwatknp mentioned this pull request Jul 15, 2023
Closed
2 tasks
@siriwatknp siriwatknp marked this pull request as ready for review July 17, 2023 05:53
@siriwatknp siriwatknp mentioned this pull request Jul 17, 2023
Open
mj12albert
mj12albert approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@mj12albert mj12albert left a comment

Choose a reason for hiding this comment

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

It works 🙌

Thanks for digging into this @siriwatknp 🙏

@siriwatknp @DanielSchiavini
Apply suggestions from code review
c574228 
Co-authored-by: Daniel Schiavini <daniel.schiavini@gmail.com>
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
siriwatknp
siriwatknp commented Jul 17, 2023
View reviewed changes
Copy link
Member Author

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks @DanielSchiavini for the final reivew!

@siriwatknp siriwatknp merged commit 093d487 into mui:master Jul 18, 2023
18 checks passed
@mj12albert mj12albert mentioned this pull request Jul 21, 2023
Closed
@mj12albert mj12albert linked an issue Jul 21, 2023 that may be closed by this pull request
Next.js 13 app directory support #34896
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielSchiavini DanielSchiavini DanielSchiavini left review comments

@mj12albert mj12albert mj12albert approved these changes

Assignees
No one assigned
Labels
examples Relating to /examples
Projects
None yet
Milestone
No milestone
4 participants
@siriwatknp @mui-bot @DanielSchiavini @mj12albert