material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Typography] Apply font properties to typography inherit variant

Open oyar99 opened this issue 3 years ago • 5 comments

Ensure that Typography components actually inherit font properties when inherit variant is used. Since the Link component is built on top of the Typography component, this also fixes #32942.

const theme = createTheme({
  typography: {
    fontFamily: "Inconsolata, Arial"
  }
});

Before:

<ThemeProvider theme={theme}>
    <Typography variant="body1">
      <Link>This is good (it's Inconsolata)</Link>
      <br />
      <Link component="button">This is not good (it's Arial)</Link>
    </Typography>
</ThemeProvider>

Now:

<ThemeProvider theme={theme}>
    <Typography variant="body1">
      <Link>This is good (it's Inconsolata)</Link>
      <br />
      <Link component="button">This is good too (it's Inconsolata)</Link>
    </Typography>
</ThemeProvider>

Fixes #32942

oyar99 avatar Jul 23 '22 20:07 oyar99

Netlify deploy preview

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

@material-ui/core: parsed: +0.10% , gzip: +0.09% @material-ui/lab: parsed: +0.19% , gzip: +0.16% @material-ui/system: parsed: +0.54% , gzip: +0.48% @mui/material-next: parsed: +0.37% , gzip: +0.32% @mui/joy: parsed: +0.10% , gzip: +0.11%

Bundle size report

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 8b6f44a66f57efff078df86c5535f9f4ecbc31a6

mui-bot avatar Aug 01 '22 10:08 mui-bot

The visual regression tests detected another issue. The sx prop doesn't override the CSS properties from the variant. Compare https://mui.com/material-ui/react-tree-view/#gmail-clone with https://deploy-preview-33621--material-ui.netlify.app/material-ui/react-tree-view/#gmail-clone.

This has worked fine before, so I wonder if it's related to the latest changes to Emotion support in the System (cc @garronej, @mnajdova).

michaldudak avatar Aug 02 '22 06:08 michaldudak

@michaldudak I don't think the problem is with the order of how things are defined, but how this variant is defined. In the demo that you linked, if you change the fontWeight with font-weight it works as expected, see https://codesandbox.io/s/xenodochial-banach-z7sojt. There is something wrong with how the variant is defined or how the value is fetched (haven't looked into too much details, just wanted to confirm it is not related to the changes done by @garronej as that would have a much bigger impact).

mnajdova avatar Aug 05 '22 12:08 mnajdova

Thanks @mnajdova for figuring this out. I am on vacation so I wasn't able to look at it.

garronej avatar Aug 05 '22 14:08 garronej

@mnajdova Thanks, I'll take a deeper look at this, then.

michaldudak avatar Aug 08 '22 07:08 michaldudak

@ZeeshanTamboli I changed a bit the fix for the sx prop, by providing the necessary changes to the sx config instead of the styleFunctionSx. The changes are related to the default theme, for e.g. they are not related to Joy UI, so it makes sense for them not to live in the styleFucntionSx intself.

mnajdova avatar Mar 14 '23 08:03 mnajdova

@ZeeshanTamboli I changed a bit the fix for the sx prop, by providing the necessary changes to the sx config instead of the styleFunctionSx. The changes are related to the default theme, for e.g. they are not related to Joy UI, so it makes sense for them not to live in the styleFucntionSx intself.

Nice! Makes sense.

ZeeshanTamboli avatar Mar 14 '23 09:03 ZeeshanTamboli

It appears that this PR may have broken the sx handling of font props? The documentation says that you can do something like this:

sx={{
    fontWeight: 'fontWeightMedium'
}}

But the code in this PR appears to only look for the prop without the fontWeight:

sx={{
   fontWeight: 'medium'
}}

Here's a codesandbox that reproduces the issue: https://codesandbox.io/s/vibrant-brook-572rnc?file=/src/App.js

You can verify that this is a regression by changing the version of @mui/material that is pulled in to 5.11.12 or earlier, and notice that using fontWeightMedium properly changes the font weight.

Here's the documentation that refers to being able to use fontWeightLight (as an example):

https://mui.com/system/getting-started/the-sx-prop/#typography

genepaul avatar Mar 16 '23 19:03 genepaul

I created an issue for this: #36542

genepaul avatar Mar 16 '23 19:03 genepaul