[Typography] Add system props by mnajdova · Pull Request #24496 · mui/material-ui

mnajdova · 2021-01-19T13:11:44Z

BREAKING CHANGES

The following classes and style overrides keys were removed: "colorInherit", "colorPrimary", "colorSecondary", "colorTextPrimary", "colorTextSecondary", "colorError", "displayInline" and "displayBlock". These props are now considered part of the system, not on the Typography component itself. If you still wish to add overrides for them, you can use the theme.components.MuiTypography.variants options. For example

const theme = createMuiTheme({
  components: {
    MuiTypography: {
-     styleOverrides: {
-       colorSecondary: {
-         marginTop: '20px',
-       },
-     },
+     variants: {
+       props: { color: "secondary" },
+       style: {
+         marginTop: '20px',
+       },
+     }],
    },
  },
});

This PR introduces the system props on the Typography component. In order for avoiding breaking changes on the color prop (which existed before on the component and is part of the system), the previous values are still supported in addition to the system values that the color prop can have. The display property was replaced in favor of the display property of the system, as the behavior should be completely identical.

Another iteration on #24485

mui-pr-bot · 2021-01-19T13:23:37Z

@material-ui/core: parsed: -0.09% 😍, gzip: -0.07% 😍
@material-ui/lab: parsed: -0.10% 😍, gzip: -0.06% 😍

Details of bundle changes

Generated by 🚫 dangerJS against a030c46

mnajdova · 2021-01-19T13:40:18Z

  [propertiesName: string]: CSSInterpolation;
 }

+export type SystemProps = PropsFor<


Extracted so that the typings can be used in both the Box, Typogrpahy and the other CSS components we'll introduce.

mnajdova · 2021-01-19T16:16:34Z

      <DialogContentText align="inherit" color="textPrimary" />
      <DialogContentText align="inherit" color="textSecondary" />
      <DialogContentText align="inherit" color="error" />
-      {/* @ts-expect-error */}


Seems like the system props are a bit too relaxed and cannot catch these errors :\

oliviertassinari

I guess we can migrate the usages of the component in the codebase in a different iteration? (Both for Box and Typography)

mnajdova · 2021-01-19T18:52:36Z

I guess we can migrate the usages of the component in the codebase in a different iteration? (Both for Box and Typography)

Definitelly, didn't want to do all those changes in just one PR :)

mnajdova · 2021-01-20T10:42:58Z

-    'textPrimary',
-    'textSecondary',
-  ]),
+  color: PropTypes /* @typescript-to-proptypes-ignore */.any,


Needed for fixing https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217632

mnajdova · 2021-01-20T10:46:52Z

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

mnajdova · 2021-01-20T15:40:15Z

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

@oliviertassinari @eps1lon I could use some help here. I spend 40 mins trying to understand what is happening but with no success 😞

eps1lon · 2021-01-20T16:02:58Z

Looking at it right now

eps1lon · 2021-01-20T16:12:20Z

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.

Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

mnajdova · 2021-01-20T16:32:52Z

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.

Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

I should have explained the breaking changes for the classes in the PR description, my bad will do it. Regarding the color prop, with https://github.com/mui-org/material-ui/pull/24496/files#diff-e28433586a43ba335e74439005786b53bce7291ed0e5062a5a13d8254775c9ebR79 I've added backward compatibility for the colors we had before. The plan is to keep this working as it was before and in the next major version v5.x deprecate the support for these custom colors and remove the transformation in v6, so basically this change regarding the color prop should not be breaking at this moment (typings should be just relaxed in terms of changes).

eps1lon · 2021-01-21T10:00:12Z

The autocomplete for the previously known color values is lost at the moment. Are unknown values for color currently accepted? That creates two problems:

no static safety against wrong values
worse migration story in the future when we change these values

Co-authored-by: Olivier Tassinari <[email protected]>

Changes applied

mnajdova commented Jan 19, 2021

View reviewed changes

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

github-actions Bot mentioned this pull request Jan 19, 2021

Update snapshots for pr/24496 eps1lon/mui-scripts-incubator#1730

Closed

mnajdova commented Jan 19, 2021

View reviewed changes

Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

oliviertassinari added the scope: typography Changes related to typography. label Jan 19, 2021

oliviertassinari reviewed Jan 19, 2021

View reviewed changes

Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

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

Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

mnajdova commented Jan 19, 2021

View reviewed changes

mnajdova marked this pull request as ready for review January 19, 2021 16:32

mnajdova requested a review from oliviertassinari January 19, 2021 16:32

oliviertassinari added the breaking change Introduces changes that are not backward compatible. label Jan 19, 2021

oliviertassinari reviewed Jan 19, 2021

View reviewed changes

Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

oliviertassinari added the type: new feature Expand the scope of the product to solve a new problem. label Jan 19, 2021

oliviertassinari mentioned this pull request Jan 19, 2021

[Grid] Add system props #24499

Merged

mnajdova commented Jan 19, 2021

View reviewed changes

Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

oliviertassinari approved these changes Jan 19, 2021

View reviewed changes

eps1lon previously requested changes Jan 20, 2021

View reviewed changes

Comment thread packages/material-ui/src/CardHeader/CardHeader.test.js Outdated

Comment thread packages/material-ui/src/CardHeader/CardHeader.test.js Outdated

Comment thread packages/material-ui/src/DialogContentText/DialogContentText.spec.tsx

ghost approved these changes Jan 20, 2021

View reviewed changes

mnajdova requested a review from eps1lon January 20, 2021 10:23

mnajdova commented Jan 20, 2021

View reviewed changes

github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 21, 2021

mnajdova mentioned this pull request Jan 21, 2021

[docs] Mention the system props when available in the API pages #24503

Merged

github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 21, 2021

mnajdova and others added 15 commits January 27, 2021 15:40

fixed tests, updated mgiration guide

3fdc9d3

framer

4a984b7

typings improvements

9f39a3f

prettier

aa0327c

ts expected error fixes

ec780e6

lint

132cdeb

Update docs/src/pages/guides/migration-v4/migration-v4.md

c879dd7

Co-authored-by: Olivier Tassinari <[email protected]>

Update docs/src/pages/guides/migration-v4/migration-v4.md

d3b9907

add system props section in the components pages

5f1bf49

review comments

e125ae6

improved typings

983d7a3

ignore color prop

a3b95e0

declared as CSS component

f7fae77

fix CI

5f56256

reproduce docs/src/pages/guides/composition/ListRouter.tsx demo

659b8ea

oliviertassinari force-pushed the feat/typography-system-props branch from 50a84c3 to 659b8ea Compare January 27, 2021 15:01

oliviertassinari added 6 commits January 27, 2021 16:08

we already have visual regression tests coving these cases

b415748

avoid having to rely on browser tests

04358c0

no need to skip JSDOM, only for inheritance

cbe4fd4

avoid skipping tests in JSDOM

d1f0013

test public modules when possible

b76d366

avoid skiping tests in JSDOM

2f4ef9e

oliviertassinari approved these changes Jan 27, 2021

View reviewed changes

oliviertassinari added 2 commits January 27, 2021 17:04

yarn docs:typescript:formatted

6fbb847

different solution

a030c46

mnajdova merged commit 2f10b4d into mui:next Jan 28, 2021

natac13 pushed a commit to natac13/material-ui that referenced this pull request Jan 30, 2021

[Typography] Add system props (mui#24496)

aeec19e

oliviertassinari mentioned this pull request Feb 6, 2021

[examples] Align more with the v5 recommended approach #24798

Merged

oliviertassinari mentioned this pull request Apr 16, 2021

[docs] Replace Typography color values with system values #25805

Merged

Uh oh!

Uh oh!

Conversation

mnajdova commented Jan 19, 2021 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BREAKING CHANGES

Uh oh!

mui-pr-bot commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnajdova Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mnajdova commented Jan 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnajdova Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

mnajdova commented Jan 20, 2021

Uh oh!

mnajdova commented Jan 20, 2021

Uh oh!

eps1lon commented Jan 20, 2021

Uh oh!

eps1lon commented Jan 20, 2021

Uh oh!

mnajdova commented Jan 20, 2021

Uh oh!

eps1lon commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mnajdova commented Jan 19, 2021 •

edited by oliviertassinari

Loading

mui-pr-bot commented Jan 19, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

eps1lon commented Jan 21, 2021 •

edited

Loading