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

[core] Fix regression broken ad on Joy #35330

Draft
wants to merge 2 commits into
base: v5.x
Choose a base branch
from
Draft

[core] Fix regression broken ad on Joy #35330

wants to merge 2 commits into from

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 4, 2022 •
edited
Loading

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 core Infrastructure work going on behind the scenes regression A bug, but worse labels Dec 4, 2022
@mui-bot
Copy link

mui-bot commented Dec 4, 2022 •
edited
Loading

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 bug 🐛 Something doesn't work 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
mnajdova
mnajdova reviewed Dec 20, 2022
View reviewed changes
docs/pages/about.tsx
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 oliviertassinari mentioned this pull request Dec 31, 2022
Merged
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 31, 2022 •
edited
Loading

@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 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova left review comments

@siriwatknp siriwatknp Awaiting requested review from siriwatknp

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged regression A bug, but worse
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliviertassinari @mui-bot @siriwatknp @mnajdova