-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
[Typography] Improve custom component types support #18868
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] Improve custom component types support #18868
Conversation
|
No bundle size changes comparing 4e07461...57c238b |
|
The changes look safe, especially with the added test case (thank you for that) and the prior work with the other components we replicate here. |
|
@fyodore82 Well done. |
|
@oliviertassinari @fyodore82 I am missing the right typings after this change. I was using Property 'component' does not exist on type 'OverrideProps<TypographyTypeMap<{}, "span">, "span">'.
11 | const props: TypographyProps = {}
> 12 | content.tag && (props.component = content.tag) |
|
@dohomi, this became a bit tricky now, but that's how it implemented in If you don't know in advance what component you'll use you can do |
| classKey: TypographyClassKey; | ||
| } | ||
|
|
||
| declare const Typography: OverridableComponent<TypographyTypeMap>; |
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 change makes the export of TypographyProps unused, does it not @fyodore82? So code that used to reference and Pick from TypographyProps I see is now broken and a mismatch for the Typography component itself.
For example:
// simple override of default variant
export const Typo = React.forwardRef<any, TypographyProps>((props, ref) => (
<Typography variant="body2" ref={ref} {...props} />
))
// valid
<Typography component="a" href="url" display="block" />
// now invalid
<Typo component="a" href="url" display="block" />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 see this is an updated pattern elsewhere in the code (e.g. Chip, Grid, Link etc), but unsure how the *Props exports relate.
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.
Pattern I've employed for Typography is used in several other components in Material-UI as you already mentioned. So I've decided not to invent different solution but follow already established mainline.
The TypographyProps and Typography itself are related. Both inherit from OverrideProps interface. Typography does it indirectly, rhru OverridableComponent and TypographyProps does it directly.
forwardRef will also work fine if specialize TypographyProps with correct component
export const Typo = React.forwardRef<any, TypographyProps<'a', {component: 'a'}>>((props, ref) => (
<Typography variant="body2" ref={ref} {...props} />
))
// now valid
<Typo component="a" href="url" display="block" />
I have also checked how it was before Typography change. This code <Typo component="a" href="url" display="block" /> was not valid due to presence of href attribute. And we had no means to make it valid.
I may suggest to extend Typography documentation and include samples of how to use component prop in different scenarios
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.
@fyodore82 So was the documentation ever updated? Because now I can't use FC<TypographyProps> anymore, which was a bit of a low blow to sneak into a minor release.
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.
Fix #18473