-
-
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
[website] Migrate blog pages to use CSS theme variables #34976
Conversation
|
@oliviertassinari The code font changes because of #34954, Are you sure about this? |
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
@siriwatknp I didn't intend this change. I have created #35027 to improve the situation. Visual regression tests could have helped avoid this regression. |
Got it, then this PR should help spot the change in the visual regression. |
…ite/blog-css-vars
…l-ui into website/blog-css-vars
textDecoration: 'none', | ||
const Root = styled('div')( | ||
({ theme }) => ({ | ||
...lightTheme.typography.body1, |
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.
Would this be better?
...lightTheme.typography.body1, | |
...theme.typography.body1, |
borderRadius: `var(--muidocs-shape-borderRadius, ${ | ||
theme.shape?.borderRadius ?? lightTheme.shape.borderRadius | ||
}px)`, |
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.
Why would theme.shape be undefined
a valid case? Should we have expected?
borderRadius: `var(--muidocs-shape-borderRadius, ${ | |
theme.shape?.borderRadius ?? lightTheme.shape.borderRadius | |
}px)`, | |
borderRadius: `var(--muidocs-shape-borderRadius, ${theme.shape.borderRadius}px)`, |
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.
But this seems to hide something else. Should we have expected?
borderRadius: `var(--muidocs-shape-borderRadius, ${ | |
theme.shape?.borderRadius ?? lightTheme.shape.borderRadius | |
}px)`, | |
borderRadius: theme.vars.shape.borderRadius, |
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.
It feels like this component's parent infrastructure wasn't ready to allow MarkdownElement.js
to be migrated to CSS vars.
...lightTheme.typography.body1, | ||
color: `var(--muidocs-palette-text-primary, ${lightTheme.palette.text.primary})`, | ||
'& strong': { | ||
color: `var(--muidocs-palette-text-primary, ${lightTheme.palette.text.primary})`, |
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.
Would this be better?
color: `var(--muidocs-palette-text-primary, ${lightTheme.palette.text.primary})`, | |
color: `var(--muidocs-palette-text-primary, ${theme.colorScheme.light.palette.text.primary})`, | |
the coupling with the imports of lightTheme
and darkTheme
seems unhealthy for design consistency because the Material UI components don't use these values.
@oliviertassinari My intention is to move I think the docs component should not be altered by the theme context. It is very hard to reason and we have seen a lot of crashes based on the theme context. This also allows the component to be rendered on the pages that haven't migrated to CSS variables. |
@siriwatknp The end goal sounds great. I'm not sure about the implementation. Would this be simpler/work?
|
@siriwatknp I have found a regression from this PR (looking a bit at https://mui-org.slack.com/archives/C042VP80PT5/p1667372692296329): The commit on master just before this PR: https://636d3dbaea7a9900083c33e1--material-ui.netlify.app/material-ui/api/alert/ This PR: https://deploy-preview-34976--material-ui.netlify.app/material-ui/api/alert/ |
part of #34880
How to check?
✅ In development, the page does not throw class names mismatch between server and client.
Changes
applyDarkStyles
in the blog pageMarkdownElement
that is used in the docs, refactor the styles to use CSS variables with fallbacks to the branding light & dark theme.Before: https://master--material-ui.netlify.app/blog/date-pickers-stable-v5/
After: https://deploy-preview-34976--material-ui.netlify.app/blog/date-pickers-stable-v5/