Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Sergio16T
Copy link
Contributor

@Sergio16T Sergio16T commented Jul 15, 2024

Fixes #42918

Typography Prop Extended to Enforce Typography Breakpoint Types.

@mui-bot
Copy link

mui-bot commented Jul 15, 2024

Netlify deploy preview

https://deploy-preview-42945--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 556ce79

@Sergio16T
Copy link
Contributor Author

Reviewed CI build step failure and am working to resolve.

@Sergio16T
Copy link
Contributor Author

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.

@ZeeshanTamboli ZeeshanTamboli added typescript component: Typography The React component. package: material-ui Specific to @mui/material labels Jul 16, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a 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.

@Sergio16T
Copy link
Contributor Author

Sergio16T commented Jul 16, 2024

@ZeeshanTamboli Thank you for the feedback on the PR! Makes sense about omitting the sx prop to reduce the scope of changes. And the same for @typescript-to-proptypes-ignore to disable the proptype generation in favor of PropTypes.object. 🚀

@ZeeshanTamboli ZeeshanTamboli added the needs cherry-pick The PR should be cherry-picked to master after merge label Jul 16, 2024
@@ -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' } }} />
Copy link
Contributor

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?

Copy link
Contributor Author

@Sergio16T Sergio16T Jul 16, 2024

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.

Copy link
Contributor

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!

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Jul 17, 2024

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.

@Sergio16T
Copy link
Contributor Author

Sergio16T commented Aug 2, 2024

@ZeeshanTamboli, @DiegoAndai, @siriwatknp Commenting to keep this PR active. Awaiting final review.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Aug 2, 2024

@ZeeshanTamboli, @DiegoAndai, @siriwatknp Commenting to keep this PR active. Awaiting final review.

Upon reflection, I believe this is a breaking change. If users extend TypographyProps in their custom components, they must also omit the sx prop if it's defined in their custom component. That would seem like a edge case though. It looks good to me, and I've already requested reviews from others.

@ZeeshanTamboli ZeeshanTamboli removed the needs cherry-pick The PR should be cherry-picked to master after merge label Aug 2, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 15, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 19, 2024
@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Sep 19, 2024

@DiegoAndai @siriwatknp Just a reminder to review this PR.

@DiegoAndai
Copy link
Member

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 typography type? So it won't accept any other values that are not the variants?

If so, I don't think this should be changed in the component level but rather the SxProps level, as it should work on all components in the same way

What do you think @siriwatknp? I'm not familiar with how the typography sx property is implemented, may I ask you to provide your insight into how it works?

@siriwatknp
Copy link
Member

While this fixes the typography but it creates more complexity to the codebase. What's about other fields? I'm pretty sure there will be more requests once this is merged.

The underlying problem is the type of sx prop, not specifically typography. It cannot be enforced to the responsive type because sx prop has to support any CSS selector which is a string.

The best experience I can achieve so far is with autocomplete for the typography but not possible to enforce the responsive keys due to https://github.com/mui/material-ui/blob/master/packages/mui-system/src/styleFunctionSx/styleFunctionSx.d.ts#L64

I put on hold to this PR until we can figure out a better solution for the whole sx type system. cc @Janpot

@siriwatknp siriwatknp added the on hold There is a blocker, we need to wait label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. on hold There is a blocker, we need to wait package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Typography] Enforce responsive typography type checking in sx prop
6 participants