-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Typography] Replace dot notation color value to work with Pigment CSS #43288
Conversation
Netlify deploy previewhttps://deploy-preview-43288--material-ui.netlify.app/ Link: parsed: +0.73% , gzip: +0.21% Bundle size reportDetails of bundle changes (Toolpad) |
I left a question in the issue #43278 (comment) |
Another option is to add export const TypographyRoot = styled('span', {
name: 'MuiTypography',
slot: 'Root',
overridesResolver: (props, styles) => {
const { ownerState } = props;
return [
styles.root,
];
},
})(({ theme }) => ({
margin: 0,
variants: [
…,
{ props: { color: 'text.primary' }, style: { color: theme.palette.text.primary },
{ props: { color: 'text.secondary' }, style: { color: theme.palette.text.secondary },
],
})); With this, it works with Pigment CSS and there is no need other internal usage of Anyway, I am fine with both of the options. |
After the discussion in #43278, I agree adding |
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.
@siriwatknp should we add a small section to the pigment migration about this?
|
Context
Reported by #43278 (comment).
The dot notation should be used in
sx
prop.For the
color
prop, the value should be a range of string so I have to addtextPrimary
andtextSecondary
to Typography because some components are usingtext.secondary
internally.Adjusted the color types to show autocomplete for supported color but still allow any string to that it does not break the app in v5.
Current change
Add
textPrimary
andtextSecondary
to Typography's color propAnother option
Add
text.primary
andtext.secondary
to the Typography's variants:With this, it works with Pigment CSS and there is no need other internal usage of
<Typography color="text.secondary">
. However, I feel that it's a bit inconsistent with other components that hascolor
prop.Anyway, I am fine with both of the options.