[Typography] Better defaults

[oliviertassinari]

Breaking changes

These are the last changes for the typography I'm proposing for stable v4:

• The font-size, change the default variant from body2 to body1. 16px is a better default than 14px. Bootstrap, material.io or even our documentation use 16px as a default font size. 14px like Ant Design is understandable as Chinese users have a different alphabet. We document 12px as the default font size for Japanese.
• The color, it should inherit most of the time. It's the default behavior of the web. I have removed the default color from the typography variants. cc @joshwooding
• Rename color="default" to color="initial" following the logic of [RFC] color prop API #13028. The usage of default should be avoided. cc @eps1lon
• The line-height, the specification doesn't mention it https://material.io/design/typography/the-type-system.html#. cc @n-batalha However, it's a key element in achieving a vertical rhythm. I have only found one change that could help align on the 4px grid, the other values look good:

-    body2: buildVariant(fontWeightRegular, 14, 1.5, 0.15),
+    body2: buildVariant(fontWeightRegular, 14, 1.43, 0.15),

[mui-pr-bot]

Details of bundle changes.

Comparing: 264c64d...d6bed5e

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.01%	-0.05%	350,243	350,218	90,024	89,983
@material-ui/core/Paper	-0.03%	-0.03%	67,867	67,847	19,823	19,817
@material-ui/core/Paper.esm	-0.03%	-0.04%	60,198	60,178	18,568	18,561
@material-ui/core/Popper	0.00%	0.00%	30,463	30,463	10,527	10,527
@material-ui/core/styles/createMuiTheme	-0.12%	-0.05%	17,384	17,364	5,729	5,726
@material-ui/core/useMediaQuery	0.00%	0.00%	2,469	2,469	1,041	1,041
@material-ui/lab	-0.01%	-0.02%	148,279	148,259	43,580	43,572
@material-ui/styles	0.00%	0.00%	53,099	53,099	15,433	15,433
@material-ui/system	0.00%	0.00%	17,136	17,136	4,525	4,525
Button	-0.02%	-0.04%	87,947	87,927	26,064	26,054
Modal	-0.02%	-0.02%	82,055	82,035	24,563	24,557
colorManipulator	0.00%	0.00%	3,232	3,232	1,299	1,299
docs.landing	0.00%	0.00%	49,958	49,958	10,837	10,837
docs.main	+0.01% 🔺	0.00%	645,407	645,450	200,517	200,525
packages/material-ui/build/umd/material-ui.production.min.js	-0.01%	0.00%	298,542	298,513	82,758	82,761

Generated by 🚫 dangerJS against d6bed5e

[ianschmitz]

Hey thanks for improving this @oliviertassinari!

I was tagged in an email notification that originally included comments referencing the changes I proposed for FormLabel and FormHelperText to use Typography to render text. Is this no longer being considered?

[joshwooding]

It’s hard to see in Argos what each of the changes affected. I can have a look later, they all sound like good changes 👍

[oliviertassinari]

@ianschmitz I have looked at it yes. I have reverted the change. The textfield already mounts a lot of components, it would add one more to the equation. I couldn't see any direct gain. I will try to apply theme.typography.x variant. But more importantly, I focus on the breaking changes here. Anything that can be handled later, should be handled later.

[oliviertassinari]

@joshwooding Argos has been incredibly useful for this pull request. I have tried to fix all the abnormal changes. I might have missed a few. If you could review it, it would be awesome. We can wait tomorrow to merge the changes. It the least we can do for important breaking changes like those.

[joshwooding]

@oliviertassinari Looking at it now it looks fine, the reason I didn’t earlier is because I was on a phone.

[oliviertassinari]

@joshwooding Thanks for double checking, we can change the solution up until v4 stable is released.

[cesardeazevedo]

@oliviertassinari Shouldn't the text color also reflects on the ThemeProvider? i mean, on the dark theme the text color is dark https://codesandbox.io/s/4w8z5mlox
Or is that intentional?

[oliviertassinari]

@cesardeazevedo Yes, you have two options. You can:

• add <CssBaseline /> (https://codesandbox.io/s/6zl48pqpyn)
• or add a top-level <Typography component="div" color="textPrimary" /> (https://codesandbox.io/s/9j3l2yy08r).

[cesardeazevedo]

Thanks so much @oliviertassinari, the impact that i see with this change is when adding nesting themes, as you can see here https://codesandbox.io/s/43q4y70nr7

Of course could be easily fixed with the prop color='textPrimary', but in my use case, i have a dark theme and lots of Dialog's which i wrap them with a light theme (i like them to be white), and components like DialogTitle has a Typography inside, which i have to wrap the childs with a Typography, this may impact some other Components that i'm still not sure yet.

[oliviertassinari]

Thanks for providing more details. Yes, you are right. The color inheritance change makes theme nesting a bit harder.

[adamscybot]

I think this has potentially caused problems with dark mode. Is the expectation that the default variant (initial) does not change colour when in a dark theme? Or should we explicitly request textPrimary for this to work?

[oliviertassinari]

@adamscybot A top-level CssBaseline component can change it or a <Typography component="div color="textPrimary" />.

[adamscybot]

@oliviertassinari So I think the main issue is that some components like ListItem do not use the typography "textPrimary" colour, and so out of the box "dark mode" type does not change these components which is a bit unexpected. I made a code sandbox of what I mean here: https://codesandbox.io/s/8qmjywo80.

I can add wrappers/theme overrides to provide the right colour downwards, but it seems like an unexpected default. Or perhaps a documentation change is needed.

[oliviertassinari]

Thank you for the reproduction. What do you think of adding the color property to the ListItem's Typography component?

[adamscybot]

Yeh that would work. I guess what we're basically saying is that if its used internally to MUI rather than directly in userland, it should use textPrimary? Are there other places its needed? To be honest, I hadn't considered that this could actually just be a props configuration issue for ListItem that's come about because of this change. I don't know if dark mode is acting differently elsewhere.

[oliviertassinari]

I'm wondering if it shouldn't be the Paper responsibility to set the color in conjunction with the background color.

--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -16,6 +16,7 @@ export const styles = theme => {
     /* Styles applied to the root element. */
     root: {
       backgroundColor: theme.palette.background.paper,
+      color: theme.palette.text.primary,
     },
     /* Styles applied to the root element if `square={false}`. */
     rounded: {

[mustafahlvc]

Giving color to Paper element is seems to me a valid solution. Like body element we are giving base color for children inheritance. This also solves the issue for body appended components like dialog, menu which are using paper.

This is another reproduction with some components and nested theming:
https://codesandbox.io/embed/32mxj2o9wm
I gave color and bg to the themes content (thinking as a body of the nested theme)

[oliviertassinari]

@mustafahlvc do you want to submit a pull request with the above fix? :)

[mustafahlvc]

Of course #15465

[adamscybot]

Awesome thanks a lot guys, I think this solution makes sense.

[acroyear]

An FYI, 4.0.2, adding CssBaseline/ only went so far. Many things were modified, but 2 were not. MuiButtonBase and MuiSnackbarContent both still had the darker foreground color (color: inherit), necessitating overrides in my Theme Creator.

Buttons being dark text was inconsistent. Some were just fine, but button for the ListItem component were always dark.

When I can, I'll go back to the above code snippets and see if I can replicate outside of my app.

(note, neither of those are directly inside Typography components)

UPDATE: It wasn't made clear that CssBaseline needs to be INSIDE the Theme Provider component. Previously, it would exist outside with no issues. That resolved the MuiButtonBase problem, though MuiSnackbarContent is still picking the darker color.