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] Update custom Typography variants example #36185
[docs] Update custom Typography variants example #36185
Conversation
Netlify deploy preview
Bundle size report |
cec67ed
to
e628a79
Compare
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.
I love to see improvements to the docs like this! π€© Keep them coming @mj12albert ! My review is all related to style. Let me know if you have any questions about style rules and best practices. :)
docs/data/material/customization/typography/TypographyCustomVariant.tsx
Outdated
Show resolved
Hide resolved
e31d02b
to
126a111
Compare
Updated β thanks for the suggestions @samuelsycamore ! |
// we have to provide the default variantMapping first because currently | ||
// custom entries will replace this whole object instead of being merged | ||
h1: 'h1', | ||
h2: 'h2', | ||
h3: 'h3', | ||
h4: 'h4', | ||
h5: 'h5', | ||
h6: 'h6', | ||
subtitle1: 'h6', | ||
subtitle2: 'h6', | ||
body1: 'p', | ||
body2: 'p', | ||
inherit: 'p', |
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.
// we have to provide the default variantMapping first because currently | |
// custom entries will replace this whole object instead of being merged | |
h1: 'h1', | |
h2: 'h2', | |
h3: 'h3', | |
h4: 'h4', | |
h5: 'h5', | |
h6: 'h6', | |
subtitle1: 'h6', | |
subtitle2: 'h6', | |
body1: 'p', | |
body2: 'p', | |
inherit: 'p', |
After checking the implementation, these lines are not needed.
Here is the proof: https://codesandbox.io/s/naughty-ives-5vorcl?file=/demo.tsx
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.
Thanks for double checking ~ I see it falls back to the default variantMapping here
docs/data/material/customization/typography/TypographyCustomVariant.tsx
Outdated
Show resolved
Hide resolved
126a111
to
72f7209
Compare
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.
Text looks good! π
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.
π Love to see docs improvement!
Follow up to: #36005 (comment)
Preview: https://deploy-preview-36185--material-ui.netlify.app/material-ui/customization/typography/#adding-amp-disabling-variants
This PR expands a little more on how to add custom Typography variants to material-ui.
I am assuming that its very likely a user would want to (or want to know how to) specify the default HTML element for custom Typography variants if they are on this part of the docs, and it's not obvious (at least I completely missed it π€¦ ) that this is the way to do it until Jun pointed it out.
The most related existing doc about this is here but it's not linked to from that page, and doesn't directly address using
variantMapping
and custom variants together β also I think a page called "Customization/Typography" should be a one-stop shop for all of this instead of having to hop to another page!