[website] Migrate blog pages to use CSS theme variables#34976
[website] Migrate blog pages to use CSS theme variables#34976siriwatknp merged 13 commits intomui:masterfrom
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.
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.
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.
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.
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.
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
applyDarkStylesin the blog pageMarkdownElementthat 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/