-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[material-ui][Typography] Enforce responsive typography type checking in sx prop #42945
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-42945--material-ui.netlify.app/ Bundle size report |
Reviewed CI build step failure and am working to resolve. |
…Link] Update interfaces sx prop type to fix failing build. Addresses components that extend Typography.
…Link] prettier formatting
…Link] prettier formatting
…Link] Alert Title SX Prop Definition Update
…Link] proptypes docs:api
Addressed many of CI failures. Could use an assist with understanding why the test_static is failing with the following prompt: pnpm proptypes` changes committed? I ran pnpm proptypes && pnpm docs:api to generate changes and committed. |
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.
@Sergio16T Thanks for the pull request. I made the necessary changes. We needed to omit the sx
prop from other component prop types that extend Typography props. Additionally, we kept the sx
propType in Typography as PropTypes.object
to avoid a large change, which works well.
Looks good to me. @DiegoAndai @siriwatknp to give a final review.
@ZeeshanTamboli Thank you for the feedback on the PR! Makes sense about omitting the |
@@ -55,6 +55,9 @@ const typographyTest = () => { | |||
<Typography component={CustomComponent} prop1="1" /> | |||
{/* @ts-expect-error */} | |||
<Typography component={CustomComponent} prop1="1" prop2="12" /> | |||
<Typography sx={{ typography: { xs: 'body1', sm: 'h2', md: 'h1', lg: 'body2' } }} /> | |||
{/* @ts-expect-error */} | |||
<Typography sx={{ typography: { xs: 'body 1', sm: 'h2', md: 'h1', lg: 'body1' } }} /> |
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'm curious if we can eventually use custom typography variants if they are re-declared in d.ts
files. E.g. will the code above throw an error with this overrides:
export interface CustomTypographyVariants {
['H1 title']?: React.CSSProperties;
['Body L']?: React.CSSProperties;
}
declare module '@mui/material/styles' {
interface TypographyVariants extends CustomTypographyVariants {}
interface TypographyVariantsOptions extends CustomTypographyVariants {}
}
declare module '@mui/material/Typography' {
interface TypographyPropsVariantOverrides {
['H1 title']: true;
['Body L']: true;
}
}
Or in other words, will TS compiler be "okay" with body 1
if we declare it beforehand?
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.
Hello @goodwin64! I believe adding custom typography variants is supported.
I added module override to unit test to confirm this and also locally tested adding the above code block in typography.d.ts
Pushed up unit test update.
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.
Awesome news! Thank you so much!
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.
@Sergio16T Thanks for the test. We handle TypeScript module augmentation tests separately because they apply globally and need isolated testing. For custom Typography variants, there's already a test file. I added this case there instead.
Edit: Even custom breakpoints are supported.
@ZeeshanTamboli, @DiegoAndai, @siriwatknp Commenting to keep this PR active. Awaiting final review. |
Upon reflection, I believe this is a breaking change. If users extend |
@DiegoAndai @siriwatknp Just a reminder to review this PR. |
Hey @Sergio16T and @goodwin64, I'm sorry for the delay in responding to this PR. It's been an intense couple of months with the v6 release 😅. Let me know if I understand correctly: the intent of #42918 is for users to be able to restraint the If so, I don't think this should be changed in the component level but rather the What do you think @siriwatknp? I'm not familiar with how the |
While this fixes the The underlying problem is the type of The best experience I can achieve so far is with autocomplete for the I put |
Fixes #42918
Typography Prop Extended to Enforce Typography Breakpoint Types.