Skip to content

[internal] Fix regression broken ad on Joy#35330

Closed
oliviertassinari wants to merge 2 commits intomui:v5.xfrom
oliviertassinari:fix-regression-ad-joy
Closed

[internal] Fix regression broken ad on Joy#35330
oliviertassinari wants to merge 2 commits intomui:v5.xfrom
oliviertassinari:fix-regression-ad-joy

Conversation

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 4, 2022

A better fix for #35685 following #35096 (comment). A solution that flattens the context: it implements https://www.notion.so/mui-org/Site-styling-architecture-e781897a38904671ba909c51dcb28f83#d7235018176745e2b067f5570e6e0197. We never get more than 2 theme providers nested:

  • _app: no more theme context: keep the playground pristine
  • first page: add the MUI brand context
  • demo: add the Material UI or Joy UI default theme.

@oliviertassinari oliviertassinari added internal Behind-the-scenes enhancement. Formerly called “core”. type: regression A bug, but worse, it used to behave as expected. labels Dec 4, 2022
@mui-bot
Copy link

mui-bot commented Dec 4, 2022

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

No bundle size changes

Generated by 🚫 dangerJS against dfaf316

@oliviertassinari oliviertassinari added the type: bug It doesn't behave as expected. label Dec 4, 2022
@oliviertassinari oliviertassinari marked this pull request as draft December 4, 2022 23:29
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 9, 2022
Comment on lines +852 to +853
<ThemeProvider>
<BrandingCssVarsProvider>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we can now merge these two components into one, to ease up the implementation. They are used together everywhere.

@siriwatknp
Copy link
Member

@oliviertassinari Let's do the quick fix in this PR (so that we can merge it) and I will open another one for removing the global ThemeProvider (it will take some time but I think it is the right way to go)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 31, 2022

@siriwatknp Ok, the regression first: #35685.

I will open another one for removing the global ThemeProvider (it will take some time but I think it is the right way to go)

👍 The PR wasn't too far from making it work, but I abandoned it. Looking at the commits, I spent 1 hour on a Sunday's night, made it work locally pushed, and went to bed, unfortunately, the CI didn't pass 😅.

@siriwatknp siriwatknp closed this Mar 14, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 23, 2024

We still need to flatten the theme providers, the whole docs structure feels completely broken right now.

I'm not aware we have a GitHub issue that tracks this, so I'm reopening to have something open that reflects this problem. Happy to have this moved to a GitHub issue though.

@siriwatknp
Copy link
Member

I am closing this one in favor of #45386

@siriwatknp siriwatknp closed this Mar 3, 2025
@oliviertassinari oliviertassinari removed the type: bug It doesn't behave as expected. label Apr 30, 2025
@oliviertassinari oliviertassinari changed the title [core] Fix regression broken ad on Joy [internal] Fix regression broken ad on Joy Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”. PR: out-of-date The pull request has merge conflicts and can't be merged. type: regression A bug, but worse, it used to behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants