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

[CssBaseline/Migration] Change in default fontSize #28477

Closed
2 tasks done
webarnes opened this issue Sep 19, 2021 · 3 comments · Fixed by #28530
Closed
2 tasks done

[CssBaseline/Migration] Change in default fontSize #28477

webarnes opened this issue Sep 19, 2021 · 3 comments · Fixed by #28530
Assignees
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@webarnes
Copy link

This is an issue with documentation.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Text 😯

One of the v5 breaking changes is #24018 and the suggested fix is:

[CssBaseline] The body font size has changed from theme.typography.body2 (0.875rem) to theme.typography.body1 (1rem).
To return to the previous size, you can override it in the theme:

const theme = createMuiTheme({
  typography: {
    body1: {
      fontSize: '0.875rem',
    },
  },
});

(Note that this will also affect use of the Typography component with the default body1 variant).

Suggested Text 🤔

It would make more sense to address this with an override to CssBaseline itself. As noted, the fix above will change body1 to be the same size as body2 throughout the app. Better to just change the fontSize on body back to the old default:

const theme = createMuiTheme({
  components: {
    MuiCssBaseline: {
      styleOverrides: {
        body: {
          fontSize: '0.875rem',
        },
      },
    },
  },
})

Possibly, to perfectly mimic the old behaviour, it should also include lineHeight and letterSpacing.

@webarnes webarnes added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 19, 2021
@TheSpookyCat
Copy link

TheSpookyCat commented Sep 20, 2021

Would it be possible to add this as part of AdaptV4Theme and thus part of the codemod? Being a breaking change surely it should be handled as I certainly didn't expect to see the change in font size after following the migration guide.

@siriwatknp siriwatknp added docs Improvements or additions to the documentation and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 21, 2021
@siriwatknp
Copy link
Member

@webarnes I agree, the approach to old behavior should be done via CSSBaseline not body1 variant.

@siriwatknp siriwatknp changed the title [CssBasline/Migration] Change in default fontSize [CssBaseline/Migration] Change in default fontSize Sep 21, 2021
@mnajdova
Copy link
Member

@webarnes would you like to create a PR for updating the migration guide? :)

@mnajdova mnajdova added the good first issue Great for first contributions. Enable to learn the contribution process. label Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants