[Button] line-height removed

[eps1lon]

@oliviertassinari I've seen that you removed the line-height. Was that intended? We noticed several UI changes in our app.

lineHeight: '1.4em', // Improve readability for multiline button.

Cheers, Thomas

Originally posted by @thomaskempel in #13136 (comment)

[eps1lon]

@thomaskempel Which variants have no more line-height? There should be no changes to their styles unless you explicitly use typography v2.

@oliviertassinari button didn't have a line-height prior to typography v2. Adding it should be considered a breaking change.

[TrySound]

There is something wrong with next typography variants flag. For example ListItemText has bigger lineHeight internally

[oliviertassinari]

button didn't have a line-height prior to typography v2. Adding it should be considered a breaking change.

@eps1lon True @thomaskempel Do you want to add the line-height back in the Button style? We should add a warning so we don't forget to remove it with v4.

There is something wrong with next typography variants flag. For example ListItemText has bigger lineHeight internally

@TrySound I agree, I think that we should use the body1 variant over the subheading variant with the next typography variants flag.

https://github.com/mui-org/material-ui/blob/fb2122b3456edc82fb59567e5d350a4db3d1c651/packages/material-ui/src/ListItemText/ListItemText.js#L65

Benchmark:

• https://material-components.github.io/material-components-web-catalog/#/component/list
• https://material.io/design/components/lists.html#specs
• https://material.io/design/typography/#

[eps1lon]

button didn't have a line-height prior to typography v2. Adding it should be considered a breaking change.

@eps1lon True @thomaskempel Do you want to add the line-height back in the Button style? We should add a warning so we don't forget to remove it with v4.

Again: What do you mean by "back"? It didn't even have a line-height in ~1.0:
https://github.com/mui-org/material-ui/blob/73101790333124cc8f5857bc46038b7d2315842a/packages/material-ui/src/styles/createTypography.js#L113-L119

@TrySound
What do you mean by "internally"? The docs use typography v2 while 3.x still uses typography v1 by default. Changes in line-height are most likely intended.

[tho-graf]

Thanks for your help. I don't want to make too much trouble. I was just wondering if it was intended. I don't use any variant.
To keep the old style I just added the the removed line (in Button.js, on the root class) to my default style in the theme until we migrate to next. Don't know how many projects are affected by the same issue.

[TrySound]

ListItemText just looks ugly with enabled next typography flag, nothing else

[oliviertassinari]

@eps1lon Here is what I have in mind:

export const styles = theme => ({
  root: {
+   lineHeight: '1.4em', // To remove after v4 is released.
    ...theme.typography.button,

          primary = (
            <Typography
+             variant={theme.typography.useNextVariants ? 'body1' : 'subheading'}
-             internalDeprecatedVariant
              className={classNames(classes.primary, { [classes.textDense]: dense })}
              component="span"
              {...primaryTypographyProps}

Does someone want to work on it?

[eps1lon]

Oh so we're not talking about the button variant but the Button component. That was not obvious from the original comment since it was posted in a PR that didn't signal any changes to the actual implementation.

I didn't agree with the change in the first place. Especially since it was targeted at multiline buttons that never appear in the material design guidelines.

@oliviertassinari
This came up previously in #12726. I originally preferred subtitle1 (which is currently used if typography v2 is enabled) but body1 and body2 are closer to the primary-secondary concept. Only difference between subtitle1 and body1 is the line-height. subtitle1 is closer to the spec mocks. The internalDeprecatedVariant is still necessary though for typography v1 users that have deprecation warnings enabled.

[oliviertassinari]

@eps1lon I'm note sure to understand. Are you happy to move forward with the changes I have proposed in #13900 (comment)?

1. Fix a recent regression.
2. Makes us follow the specification more closely. You can check you the reference implementation or the specification website. The two line spacing is too wide by 4px, exactly what we get with moving from subtitle1 to body1 (1.75 * 16 - 1.50 * 16 = 4). Oh, I have forgotten about Can we put ListItemText and table body cell text in a font size category of their own or use body2/body1? #12726! It's a strong signal then.

Regarding the multiline button, I think that it's important to handle, it can happen independently from the developer will. For instance went translating the website, or when resizing the screen.