[Typography] Migrate styles to emotion #23841

DanailH · 2020-12-04T15:02:28Z

This PR migrates the Typography component to use the v5 styled engine. Steps necessary for the changes.

Convert the styles definition to use experimentalStyled for each of the slots in the component (we only have the Root here).
Create overridesResolver function that will be used as a parameter for the experimentalStyled utility for the Root that will map how the styles overrides will be added to the styles of the component, based on the provided props.
Create useComponentNameClasses hook, that will append the correct global and input classes on the component based on the props. We recommend you extract the global classes under a componentNameClasses object, that can be further used by developers and for updating the tests.
Use the useThemeProps hook, for getting default props from he theme.components options.
const props = useThemeProps({ props: inProps, name: 'MuiTypography' });
Map all state and props to the stylesProps key, and use this in the experimentalStyled utility.
Finally, if the component implemented the component prop, use emotion's (styled-components's) as prop for forwarding this prop.

For updating the tests, your ill need to do the following:

Replace the describeConformance with describeConformanceV5. You will need in addition to provide the following options: muiName and testVariantProps for testing the theme.components options on the updated component.
Import and use the componentnameClasses object and use it trough out the tests, whenever the classes are required.

mui-pr-bot · 2020-12-04T15:05:02Z

@material-ui/core: parsed: +0.28% , gzip: +0.39%
@material-ui/lab: parsed: +7.06% , gzip: +10.81%

Details of bundle changes

Generated by 🚫 dangerJS against ff81d3e

mnajdova · 2020-12-04T16:14:44Z

docs/src/modules/components/Link.js

@@ -8,17 +8,17 @@ import MuiLink from '@material-ui/core/Link';
 import { useUserLanguage } from 'docs/src/modules/utils/i18n';

 const NextComposed = React.forwardRef(function NextComposed(props, ref) {
-  const { as, href, ...other } = props;
+  const { linkAs, href, ...other } = props;


We had conflict here with the as prop from emotion.

I think we should document the props that are now intercepted by emotion. It is a breaking change if you were passing these through. Maybe add a section in the changelog that only lists affected components and once we migrated all we just give the general advise that if you had <MuiComponent component={SomeOtherComponent} emotionProp1 emotionProp2 />, that these are no longer passed to SomeOtherComponent

Will we need to update the Next.js and Gatsby examples for the conflict?

@eps1lon will add an entry in the migration-v4.md for it. @oliviertassinari good call, let me check if the examples need to be updated.

Added migration entry - d764665 we can extend it with more props if there are any. At this moment this is the only one I could notice not being propagated.

Both nextjs examples have been updated. I didn't noticed anything worth updating in the gatsby examples.

packages/material-ui/src/Typography/Typography.js

mnajdova · 2020-12-04T16:20:05Z

packages/material-ui/src/Typography/Typography.js

+  'span',
+  {},
+  { muiName: 'MuiTypography' },
+)((props) => ({


I like how the styles are much more compact :)

To be fair, most of the reduction comes from the missing inline documentation. Last time I asked, core members did seem to rely on them so I'm not sure the new compactness a good thing (can't find the poll in slack anymore). Personally, I'm happy with getting rid of them to simplify documentation but there are some unintended side-effects to this change.

Agree, that's true too, but still I like how all variants are simply defined by one line:

...(props.styleProps.variant && props.theme.typography[props.styleProps.variant]),

at the same time allowing us to support dynamic variants out of the box :)

mnajdova · 2020-12-04T16:22:08Z

packages/material-ui/src/Typography/Typography.js

+  { muiName: 'MuiTypography' },
+)((props) => ({
+  margin: 0,
+  ...(props.styleProps.variant && props.theme.typography[props.styleProps.variant]),


With this line we are enabling direct variant override for something defined in the theme 🚀 This has been a feature requested in #22257

packages/material-ui/src/Typography/Typography.js

mnajdova · 2020-12-07T17:12:30Z

Note to me: after 0cad18a the proptypes generation is not including the className and component props. :\

oliviertassinari

@oliviertassinari now that we do not use the withStyles wrapper I guess we need to have these tests per component.

@mnajdova Makes sense.

Considering the last feedback from @eps1lon, I think that it's interesting that we only migrate the component, without handling the documentation nor the unstyled component. How about we use the Typography to determine the best course of action? We try to do the changes in isolation. I have to agree with Sebastian that if we aim to ask for help from the community, the smaller the migration tasks are, the most likely they will be successful.

But maybe we don't even need an unstyled Typography component?

eps1lon · 2020-12-07T19:06:57Z

Known issues: currently the as prop is not respected.

Why is this an issue? We already have the component prop which does the same thing. I don't think we want to have two props for the same concern.

mnajdova · 2020-12-07T19:37:48Z

Why is this an issue? We already have the component prop which does the same thing. I don't think we want to have two props for the same concern.

Yep, it was fixed, we use the component prop, but leverage the as prop from emotion/styled-components for generating the correct tag

eps1lon

Something seems off about the styling approach.

For example, the <nav> in https://deploy-preview-23841--material-ui.netlify.app/components/breadcrumbs/#basic-breadcrumbs now has one additional class (css-znp3um-MuiTypography for me). In addition, only one of the Typography related classes has a declaration:

MuiTypography-root - No styles applied
MuiTypography-colorTextSecondary - No styles applied
MuiTypography-body1 - No styles applied
MuiBreadcrumbs-root - No styles applied
css-znp3um-MuiTypography - Styles applied

Before only MuiBreadcrumbs-root had no styles applied (because it is empty).

The reason I find this problematic: It is now not apparent from the devtools which styles come from which class (necessary information since
we consider CSS classes and their names part of the public API). We would now need documentation for our styles declaration and the source code is way too syntax/API heavy.

eps1lon

Something seems off about the styling approach.

For example, the <nav> in https://deploy-preview-23841--material-ui.netlify.app/components/breadcrumbs/#basic-breadcrumbs now has one additional class (css-znp3um-MuiTypography for me). In addition, only one of the Typography related classes has a declaration:

MuiTypography-root - No styles applied
MuiTypography-colorTextSecondary - No styles applied
MuiTypography-body1 - No styles applied
MuiBreadcrumbs-root - No styles applied
css-znp3um-MuiTypography - Styles applied

Before only MuiBreadcrumbs-root had no styles applied (because it is empty).

The reason I find this problematic: It is now not apparent from the devtools which styles come from which class (necessary information since
we consider CSS classes and their names part of the public API). We would now need documentation for our styles declaration and the source code is way too syntax/API heavy.

mnajdova · 2020-12-11T17:19:10Z

@mnajdova I'm not entirely sure the solution has to be found on the visual regression runner. Look at this demo: https://deploy-preview-23841--material-ui.netlify.app/components/breadcrumbs/#custom-separator once opened in codesandbox: https://codesandbox.io/s/tk3hz?file=/demo.js. There is likely something to do about it for developers too.

Will be fixed by #23934

mnajdova · 2020-12-15T10:51:21Z

CSS injection was fixed by #23934

packages/material-ui/src/Typography/Typography.test.js

oliviertassinari · 2020-12-15T18:33:12Z

docs/src/modules/components/Link.js

@@ -8,17 +8,17 @@ import MuiLink from '@material-ui/core/Link';
 import { useUserLanguage } from 'docs/src/modules/utils/i18n';

 const NextComposed = React.forwardRef(function NextComposed(props, ref) {
-  const { as, href, ...other } = props;
+  const { linkAs, href, ...other } = props;


Will we need to update the Next.js and Gatsby examples for the conflict?

test/utils/describeConformanceV5.js

oliviertassinari · 2020-12-15T18:37:13Z

packages/material-ui/src/Typography/Typography.js

+  { muiName: 'MuiTypography' },
+)((props) => ({
+  margin: 0,
+  ...(props.styleProps.variant && props.theme.typography[props.styleProps.variant]),


oliviertassinari · 2020-12-15T18:47:40Z

packages/material-ui/src/Typography/Typography.js

-  noWrap: {
+const getTextColor = (color, palette) => {
+  if (color.indexOf('text') === 0) {
+    return palette.text[color.split('text').pop().toLowerCase()];


In the future, we probably want to change the value supported by the color prop to match the format of https://github.com/mui-org/material-ui/blob/982abc5771c1ce2e0508d9bc79ad0bb9d43743d2/packages/material-ui-system/src/style.js#L5

<Typograpy color="text.secondary" />

Or we could also say, drop the prop for the sx prop instead. It won't be consistent with the color prop of a button but that would be consistent with the other sx usages.

<Typograpy sx={{ color: 'text.secondary' }} />

Sounds good to me, let’s review the color prop on all components and see what patterns are there and make a proposal fo API. Sounds good?

Yeah, it will be for a future iteration. It's outside of the scope of this effort.

First draft for typography migration to emotion

8bdd48c

DanailH requested a review from mnajdova December 4, 2020 15:02

DanailH and others added 2 commits December 4, 2020 16:12

Fix formatting

e844fc4

fixes

565cd4f

mnajdova reviewed Dec 4, 2020

View reviewed changes

packages/material-ui/src/Typography/Typography.js Outdated Show resolved Hide resolved

mnajdova reviewed Dec 4, 2020

View reviewed changes

mnajdova added 2 commits December 7, 2020 16:55

fixes

2f95356

fixed tests

8ae6f0d

github-actions bot mentioned this pull request Dec 7, 2020

Update snapshots for pr/23841 eps1lon/mui-scripts-incubator#1460

Closed

prettier

1d3cc1d

mnajdova reviewed Dec 7, 2020

View reviewed changes

packages/material-ui/src/Typography/Typography.js Outdated Show resolved Hide resolved

default props

0cad18a

This comment has been minimized.

Sign in to view

fix build

eb9e493

oliviertassinari reviewed Dec 7, 2020

View reviewed changes

fixes

5540769

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 8, 2020

mnajdova added 2 commits December 8, 2020 10:54

Merge branch 'next' into feature/Typography-switch-to-emotion

859ca7f

prettier

f8c9934

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 8, 2020

mnajdova mentioned this pull request Dec 8, 2020

[docs] Add prepend option on emotion caches to allow JSS style overrides #23892

Merged

eps1lon reviewed Dec 8, 2020

View reviewed changes

Check if color has "text" to cover more colors

158a5ff

docs:api

a656da8

Merge branch 'next' into feature/Typography-switch-to-emotion

d9d73ed

mnajdova requested review from mbrookes and oliviertassinari December 15, 2020 10:51

updated tests

5c5e764

vercel bot deployed to Preview December 15, 2020 12:32 View deployment

eps1lon requested changes Dec 15, 2020

View reviewed changes

packages/material-ui/src/Typography/Typography.test.js Show resolved Hide resolved

packages/material-ui/src/Typography/Typography.test.js Outdated Show resolved Hide resolved

packages/material-ui/src/Typography/Typography.test.js Show resolved Hide resolved

Fix ttp not detecting input props

3280345

vercel bot deployed to Preview December 15, 2020 13:50 View deployment

mnajdova added 2 commits December 15, 2020 17:15

comment

25ec283

prettier

66a81e8

mnajdova requested a review from eps1lon December 15, 2020 16:57

eps1lon approved these changes Dec 15, 2020

View reviewed changes

oliviertassinari reviewed Dec 15, 2020

View reviewed changes

reverted describeConformance test changes

40b9ec5

mnajdova requested a review from oliviertassinari December 15, 2020 19:55

oliviertassinari approved these changes Dec 15, 2020

View reviewed changes

mnajdova added 2 commits December 16, 2020 13:25

examples updated

c42898f

added migration guide

d764665

mnajdova approved these changes Dec 16, 2020

View reviewed changes

mnajdova added 3 commits December 16, 2020 13:49

sorted props

e73af0f

Merge branch 'next' into feature/Typography-switch-to-emotion

928f85d

updated test

ff81d3e

DanailH merged commit 259253d into mui:next Dec 18, 2020

oliviertassinari mentioned this pull request Dec 18, 2020

[theme] Allow custom typography variants #22257

Closed

oliviertassinari changed the title ~~[Typography] Migrate styles to Emotion~~ [Typography] Migrate styles to emotion Dec 21, 2020

This comment has been minimized.

Sign in to view

zannager added the component: Typography The React component. label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Typography] Migrate styles to emotion #23841

[Typography] Migrate styles to emotion #23841

DanailH commented Dec 4, 2020 •

edited by mnajdova

mui-pr-bot commented Dec 4, 2020 •

edited

mnajdova Dec 4, 2020

eps1lon Dec 15, 2020

oliviertassinari Dec 15, 2020

mnajdova Dec 15, 2020

mnajdova Dec 16, 2020

mnajdova Dec 4, 2020

eps1lon Dec 15, 2020

mnajdova Dec 15, 2020

mnajdova Dec 4, 2020

oliviertassinari Dec 15, 2020

This comment has been minimized.

mnajdova commented Dec 7, 2020

oliviertassinari left a comment •

edited

eps1lon commented Dec 7, 2020

mnajdova commented Dec 7, 2020

eps1lon left a comment •

edited by oliviertassinari

eps1lon left a comment •

edited by oliviertassinari

mnajdova commented Dec 11, 2020

mnajdova commented Dec 15, 2020

oliviertassinari Dec 15, 2020

oliviertassinari Dec 15, 2020

oliviertassinari Dec 15, 2020

mnajdova Dec 15, 2020

oliviertassinari Dec 15, 2020

This comment has been minimized.

[Typography] Migrate styles to emotion #23841

[Typography] Migrate styles to emotion #23841

Conversation

DanailH commented Dec 4, 2020 • edited by mnajdova

mui-pr-bot commented Dec 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

mnajdova commented Dec 7, 2020

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

eps1lon commented Dec 7, 2020

mnajdova commented Dec 7, 2020

eps1lon left a comment • edited by oliviertassinari

Choose a reason for hiding this comment

eps1lon left a comment • edited by oliviertassinari

Choose a reason for hiding this comment

mnajdova commented Dec 11, 2020

mnajdova commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

DanailH commented Dec 4, 2020 •

edited by mnajdova

mui-pr-bot commented Dec 4, 2020 •

edited

oliviertassinari left a comment •

edited

eps1lon left a comment •

edited by oliviertassinari

eps1lon left a comment •

edited by oliviertassinari