-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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] Add custom variants support #22006
Conversation
@@ -35,6 +36,8 @@ export const styles = (theme) => ({ | |||
subtitle2: theme.typography.subtitle2, | |||
/* Styles applied to the root element if `variant="overline"`. */ | |||
overline: theme.typography.overline, | |||
/* Styles applied to the root element if `variant="inherit"`. */ | |||
inherit: {}, |
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.
For consistency I've added this variant classKey
. This way we don't need to update the warnings as the variant classKey will always exists. If we don't want to make this change, I can work on the warning.
@@ -370,150 +370,4 @@ describe('<Button />', () => { | |||
expect(markup.text()).to.equal('Hello World'); | |||
}); | |||
}); | |||
|
|||
describe('custom theme variants', () => { |
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.
This is already covered in useThemeVariants.test.js
, no need for per component tests
Co-authored-by: Olivier Tassinari <[email protected]>
subtitle1: PropTypes.string, | ||
subtitle2: PropTypes.string, | ||
}), | ||
variantMapping: PropTypes /* @typescript-to-proptypes-ignore */.object, |
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.
With adding new variants new keys can be added here, so this was relaxed to object
.
framer/scripts/framerConfig.js
Outdated
@@ -314,6 +314,7 @@ export const componentSettings = { | |||
'gutterBottom', | |||
'internalDeprecatedVariant', | |||
'paragraph', | |||
'varaint', |
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.
There's a typo but why do we want to ignore it? Seems pretty important for prototyping to me.
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.
The variants is now a union of enums + string, my understanding was that we are excluding those props.
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.
my understanding was that we are excluding those props.
Why?
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?
I believe we would need to fix the Framer generation script, not sure it's worth it:
Co-authored-by: Olivier Tassinari <[email protected]>
This comment has been minimized.
This comment has been minimized.
That use case is supported, the syntax is a bit different. const theme = createMuiTheme({
variants: {
MuiTypography: [
{
props: { variant: 's8' },
styles: {
fontSize: 14,
lineHeight: '20px',
// other styles...
},
},
],
},
}); And than you can use: <Typography variant="s8">Hello world</Typography> Here is a docs example: https://next.material-ui.com/customization/theme-components/#adding-new-component-variants that describes this API for the |
Thanks for sharing this. I wonder for future versions, if it would make sense to consolidate everything under |
@cristobalchao this is definitely something we want to explore. We are looking into different ways of using dynamic styles based on props. Once we solve that issue, this can be easily implemented. |
@cristobalchao I believe it will require the same solution to the dynamic color issue #13875. While #15573 focus on allowing the support of new props, right from the theme, #13875 goes a step further. It's meant to bypass the design tokens -> style transformation step. Developers could leverage the design tokens of the theme directly. |
Thanks for making this update! However, I'm still seeing the prop type warning in my console. I wonder if anyone else is seeing the same. |
@mikeyamato can you please ensure that you are on the latest v5 alpha version? This should not happen as the proptypes is basically relaxed to accept any string. If this is still the case for you, please create an issue. |
@mnajdova totally makes sense. Literally moments before you responded I noticed that this wasn't part of |
Quick update from my end. I tried to upgrade to v5 but that resulted in various dependency issues with my existing project. Unfortunately I won't be able to take advantage of this feature just quite yet. But looking forward to utilizing it in the near future. Till then I'll need to live with the errors 😢 . |
@mikeyamato What dependency issues did you face? |
FYI the docs example is here now: |
@EliRobinson For the typography variant, there is even simpler: https://next.material-ui.com/customization/typography/#adding-amp-disabling-variants. |
This PR adds support for custom variants in the
Typography
component as part of #21749