mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
93.03k stars 32.05k forks source link

[material-ui][Typography] Enforce responsive typography type checking in `sx` prop #42918

Open goodwin64 opened 1 month ago

goodwin64 commented 1 month ago

Summary

Here the typography shorthand has a string type as it's later being spread into the element.

We use responsive typography as in the following example:

<Typography
  sx={{
    typography: {
      xs: 'Body S',
      sm: 'Body M',
      md: 'any string is valid here',
    },
  }}
>
  The size of this text changes based on the screen size. Check it out! <br />
  In most cases, we will reuse the same typography variant across breakpoints. But
  in case you need to override it, refer to the code in this example.
</Typography>

and there's no way for TypeScript to catch the types inconsistency early in the process.

I tried a bunch of things to redeclare typography prop but with no luck. E.g.:

// attempt 1
declare module '@mui/system' {
  interface SxProps<Theme = MUITheme> extends MUISxProps<Theme> {
    typography?: {
      [key in Breakpoint]?: FlashPackTypographyVariant;
    } | FlashPackTypographyVariant;
  }
}

// attempt 2
declare module '@mui/system' {
  interface TypographyPropsVariantOverrides {
    // Your custom typography variants (as before)
  }

  interface AliasesCSSProperties {
    // Add your custom property here
    typography?: {
      [key in Breakpoint]?: FlashPackTypographyVariant;
    } | FlashPackTypographyVariant;
  }

  // This ensures the custom property is recognized in the theme
  interface ThemeOptions {
    typography?: TypographyOptions | ((theme: Theme) => TypographyOptions);
  }
}

// attempt 3
declare module '@mui/material/styles' {
  interface CSSProperties {
    typography?: {
      [key in Breakpoint]?: FlashPackTypographyVariant;
    } | FlashPackTypographyVariant;
  }
}

I'd love to get some tips if there's a way to override typography in this case!

Examples

No response

Motivation

No response

Search keywords: typography, typescript, override, generic

ZeeshanTamboli commented 1 month ago

Instead of allowing type overrides in the sx prop's typography property, we should enforce type checking. For the Typography component, we can make the following changes:

--- a/packages/mui-material/src/Typography/Typography.d.ts
+++ b/packages/mui-material/src/Typography/Typography.d.ts
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import { OverridableStringUnion } from '@mui/types';
-import { SxProps, SystemProps } from '@mui/system';
+import { SxProps, SystemProps, Breakpoint } from '@mui/system';
 import { Theme } from '../styles';
 import { OverrideProps, OverridableComponent } from '../OverridableComponent';
 import { Variant } from '../styles/createTypography';
@@ -43,7 +43,9 @@ export interface TypographyOwnProps extends SystemProps<Theme> {
   /**
    * The system prop that allows defining system overrides as well as additional CSS styles.
    */
-  sx?: SxProps<Theme>;
+  sx?: SxProps<Theme> & {
+    typography?: string | Partial<Record<Breakpoint, TypographyProps['variant']>>;
+  };
   /**
    * Applies the theme typography styles.
    * @default 'body1'

We cannot implement this in AliasesCSSProperties because the Variant type from Material UI is not available in the MUI System. While we may need this change for all sx types across all Material UI components, applying it to the Typography component is a good start.

In-case anybody is creating a new PR, make sure to add a TypeScript test case.

Sergio16T commented 1 month ago

Hello @ZeeshanTamboli! I'm beginning to work on a fix for this issue. Could you please assign it to me?

ZeeshanTamboli commented 1 month ago

@Sergio16T Assigned!

Sergio16T commented 1 month ago

Hello @ZeeshanTamboli! I opened a PR with fix. Ready for review

rkumar261 commented 1 week ago

Is this issue already resolved? If not, I'm interested in working on it.

Sergio16T commented 1 week ago

Hi @rkumar261 the dev work is complete. PR is awaiting final review from maintainers.

rkumar261 commented 1 week ago

Ok @Sergio16T, thanks