[Typography] Improve the upgrade story #13214

oliviertassinari · 2018-10-11T23:32:51Z

I wanted to address the following pain points:

The warning spam in the console
The warnings threw in the examples
The warning globally disabled in the tests
The MuiThemeProvider requirement for correctly using the new typography variants
The process.env issue
The warning hard to find source location

I had to introduce a chainPropType helper along the way. Also, I have removed the option to globally disable the warnings.

Closes #13164
Closes #13163
Closes #13175

oliviertassinari · 2018-10-11T23:38:47Z

.size-limit.js

@@ -16,13 +16,13 @@ module.exports = [
    name: 'The initial cost paid for using one component',
    webpack: true,
    path: 'packages/material-ui/build/Paper/index.js',
-    limit: '18.2 KB',
+    limit: '17.6 KB',


eps1lon

I pushed a solution for chainPropTypes that doesn't use private vars from prop-types and follows the documented pattern for calling propTypes directly.

eps1lon · 2018-10-12T05:44:52Z

test/utils/setup.js

@@ -1,4 +1,6 @@
 const createDOM = require('./createDOM');

+// eslint-disable-next-line no-underscore-dangle
+global.__MUI_USE_NEW_TYPOGRAPHY_VARIANTS__ = true;


I had such a tunnel vision from all this "functional everywhere" that I totally forgot that global exists. Great call 👍

eps1lon · 2018-10-12T05:47:03Z

docs/src/pages/style/typography/typography.md

@@ -74,9 +74,11 @@ const theme = createMuiTheme({
 });
 ```

+or set `window.__MUI_USE_NEW_TYPOGRAPHY_VARIANTS__ = true;` if you don't use the theme.


-window.__MUI_USE_NEW_TYPOGRAPHY_VARIANTS__ +window.__MUI_USE_NEXT_TYPOGRAPHY_VARIANTS__

for consistency.

Also we use global internally but window in the docs. window is not defined outside of browser, right? Maybe include global here before people ask.

This option is targeting newcomers, they won't know what global is, hence window.

eps1lon · 2018-10-12T05:49:31Z

packages/material-ui/src/styles/createTypography.js

-    suppressDeprecationWarnings = process.env.MUI_SUPPRESS_DEPRECATION_WARNINGS,
-    useNextVariants = false,
+    // eslint-disable-next-line no-underscore-dangle
+    useNextVariants = false || global.__MUI_USE_NEW_TYPOGRAPHY_VARIANTS__,


This will always evaluate to just global.__MUI_USE_NEW_TYPOGRAPHY_VARIANTS__

eps1lon · 2018-10-12T05:52:07Z

packages/material-ui/src/styles/createTypography.js

@@ -27,27 +25,18 @@ export default function createTypography(palette, typography) {
    // Tell Material-UI what's the font-size on the html element.
    // 16px is the default font-size used by browsers.
    htmlFontSize = 16,
-    suppressDeprecationWarnings = process.env.MUI_SUPPRESS_DEPRECATION_WARNINGS,


We could leave this in with global for users that want 3.2 without typography v2. I feel way to nostalgic about error_reporting(E_ALL ^ E_DEPRECATED), sorry.

We log a single warning now. I believe we don't need to hide it :)

Yes, but each one of my unit test runs standalone - so now without this escape hatch my unit test output screen seems like something is definitely wrong.

Yes, but each one of my unit test runs standalone

@jmcpeak Let's look into that. Are you using v3.2.2?

eps1lon · 2018-10-12T06:17:39Z

packages/material-ui/src/styles/createTypography.js

-    useNextVariants ||
-      !Object.keys(other).some(variant => typographyMigration.restyledVariants.includes(variant)),
-    'Material-UI: You are passing a variant to createTypography ' +
-      'that will be restyled in the next major release, without indicating that you ' +


That warning is now completely removed.

Yes, it was. It's not compatible with theme nesting.

eps1lon · 2018-10-12T06:40:34Z

pages/api/typography.md

@@ -29,7 +29,57 @@ import Typography from '@material-ui/core/Typography';
 | <span class="prop-name">internalDeprecatedVariant</span> | <span class="prop-type">bool |   | A deprecated variant is used from an internal component. Users don't need a deprecation warning here if they switched to the v2 theme. They already get the mapping that will be applied in the next major release. |
 | <span class="prop-name">noWrap</span> | <span class="prop-type">bool | <span class="prop-default">false</span> | If `true`, the text will not wrap, but instead will truncate with an ellipsis. |
 | <span class="prop-name">paragraph</span> | <span class="prop-type">bool | <span class="prop-default">false</span> | If `true`, the text will have a bottom margin. |
-| <span class="prop-name">variant</span> | <span class="prop-type">enum:&nbsp;'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'subtitle1', 'subtitle2', 'body1', 'body2', 'caption', 'button', 'overline', 'srOnly', 'inherit', "display4", 'display3', 'display2', 'display1', 'headline', 'title', 'subheading'<br> |   | Applies the theme typography styles. Use `body1` as the default value with the legacy implementation and `body2` with the new one. |
+| <span class="prop-name">variant</span> | <span class="prop-type">chainPropType(


We need to update the API docs generator.

I will finish that today. I did something like that 3 years ago, should be able to do it now.

eps1lon · 2018-10-12T06:45:51Z

packages/material-ui/src/utils/chainPropType.js

@@ -0,0 +1,18 @@
+function chainPropType(propType, check) {


I would vote for the reverse argument order since we're calling check first.

What do you think about using ...validators and then just iterating over them until we find the first error?

I agree with reversing the order if it works with documentation generation.

oliviertassinari · 2018-10-12T12:27:53Z

packages/material-ui/src/utils/chainPropType.test.js

+  let warning;
+
+  beforeEach(() => {
+    warning = mock(console).expects('error');


I remember now why we didn't do it this way. we can no longer use console.log. Let's us our helper. It's really frustrating for debugging.

oliviertassinari · 2018-10-12T12:33:27Z

@eps1lon Thank you for the review! I should be able to complete the effort this evening.

oliviertassinari · 2018-10-12T22:54:12Z

packages/material-ui/src/utils/chainPropType.test.js

+  });
+
+  it('should not warn for supported types', () => {
+    PropTypes.checkPropTypes(


Our tests are no longer idempotent with this method. The warning only displays once.

Fixed it. See facebook/react#4302. Solution inspired by facebook/react#7047 (comment)

updated

oliviertassinari · 2018-10-13T14:24:47Z

It should be good. I hope we won't have to do another iteration. We will see. @eps1lon Thank you for the review, I have tried to leverage all your feedbacks.

oliviertassinari added new feature New feature or request component: Typography The React component. labels Oct 11, 2018

oliviertassinari force-pushed the typography-upgrade-path branch 2 times, most recently from 270535b to 080be09 Compare October 11, 2018 23:38

oliviertassinari commented Oct 11, 2018

View reviewed changes

oliviertassinari requested a review from eps1lon October 11, 2018 23:38

eps1lon previously requested changes Oct 12, 2018

View reviewed changes

eps1lon reviewed Oct 12, 2018

View reviewed changes

oliviertassinari commented Oct 12, 2018

View reviewed changes

oliviertassinari force-pushed the typography-upgrade-path branch 3 times, most recently from c14a10f to 0fd97e4 Compare October 13, 2018 09:43

oliviertassinari force-pushed the typography-upgrade-path branch 3 times, most recently from 838286b to d4995aa Compare October 13, 2018 10:47

oliviertassinari and others added 2 commits October 13, 2018 16:02

[Typography] Improve the upgrade story

522c041

[core] Use documented direct propTypes call

f8be00d

oliviertassinari force-pushed the typography-upgrade-path branch from d4995aa to 737a434 Compare October 13, 2018 14:12

let's complete the effort

18c3925

oliviertassinari force-pushed the typography-upgrade-path branch from 737a434 to 18c3925 Compare October 13, 2018 14:16

oliviertassinari merged commit 118d339 into mui:master Oct 13, 2018

oliviertassinari deleted the typography-upgrade-path branch October 13, 2018 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Typography] Improve the upgrade story #13214

[Typography] Improve the upgrade story #13214

oliviertassinari commented Oct 11, 2018 •

edited

Loading

oliviertassinari Oct 11, 2018

eps1lon left a comment

eps1lon Oct 12, 2018

eps1lon Oct 12, 2018

oliviertassinari Oct 12, 2018 •

edited

Loading

eps1lon Oct 12, 2018

eps1lon Oct 12, 2018

oliviertassinari Oct 12, 2018

jmcpeak Oct 16, 2018

oliviertassinari Oct 16, 2018

eps1lon Oct 12, 2018

oliviertassinari Oct 12, 2018

eps1lon Oct 12, 2018

oliviertassinari Oct 12, 2018

eps1lon Oct 12, 2018

oliviertassinari Oct 12, 2018

oliviertassinari Oct 12, 2018 •

edited

Loading

oliviertassinari commented Oct 12, 2018

oliviertassinari Oct 12, 2018

eps1lon Oct 13, 2018

oliviertassinari commented Oct 13, 2018

[Typography] Improve the upgrade story #13214

[Typography] Improve the upgrade story #13214

Conversation

oliviertassinari commented Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Oct 13, 2018

oliviertassinari commented Oct 11, 2018 •

edited

Loading

oliviertassinari Oct 12, 2018 •

edited

Loading

oliviertassinari Oct 12, 2018 •

edited

Loading