-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs-infra] Restructure docs theme context to CSS variables #45386
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-45386--material-ui.netlify.app/ Bundle size report |
baa6e59
to
97b90da
Compare
|
||
export function resetDocsColor() { | ||
if (typeof document !== 'undefined') { | ||
document.documentElement.removeAttribute('style'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure there are no other styles added on this element? Would id be better to create a style tag with specific id and remove it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's a quick approach to test if it works.
<SettingsIcon fontSize="small" /> | ||
</IconButton> | ||
</Tooltip> | ||
<BrandingCssVarsProvider {...themeOptions}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if there are other places where this needs to be done? This AppFrame as far as I know is used only on subset of the pages. (I may be wrong with this assumption)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I checked. AppFrame only used on demo pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's the plan to add this on the other pages as well directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are all added. Marketing, blog, and API pages already contain BrandingCssVarsProvider.
Might need to adjust the API pages a bit because it's also using AppFrame.
…rial-ui into docs-infra/demo-cssvars2
…-infra/demo-cssvars2
This reverts commit 48de15a.
…-infra/demo-cssvars2
…-infra/demo-cssvars2
…-infra/demo-cssvars2
Part of #44534
Preview: https://deploy-preview-45386--material-ui.netlify.app/
This PR aims to fix 2 things:
Not possible to split the PR because they are dependent (the
_app.js
ThemeProvider has to be removed)Root cause
Dark mode flicker
This is because component (demo) pages are not using CSS variables
Toggle speed
Switching between light and dark mode cause the whole page to rerender.
This is not only about using CSS variables as you can see in the recording below.
The homepage, using CSS variables, still rerender all components because the theme values changes between
light
anddark
modes (thetheme.palette.mode
value).This cause component to rerender because all Material UI is built with
styled
which subscribe to the ThemeContext.Screen.Recording.2568-02-25.at.16.04.19.mov
Solution
The main changes of this PR are:
_app.js
ThemeProvider that causes the change between modes and migrate to CSS theme variables within each page.Screen.Recording.2568-02-25.at.16.03.36.mov
Side benefits