New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Typography] Add typography v2 variants #12916
Conversation
I might have a solution for the issue with the karma test. Will push this tomorrow. |
Argos reports only visual changes in the docs which is expected since they already use v2. Everything else looks the same which was the goal 👍 I'm going to add a migration guide and link to it in the warnings and then this is good to go. |
4f85b53
to
741dad7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, just a couple of things I spotted.
// Apply the CSS properties to all the variants. | ||
allVariants, | ||
...other | ||
} = typeof typography === 'function' ? typography(palette) : typography; | ||
|
||
warning( | ||
!Object.keys(other).some(variant => deprecatedVariants.includes(variant)), | ||
'Deprecation Warning: Material-UI: Your are passing a deprecated variant to ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"You are"
|
||
warning( | ||
useNextVariants || !Object.keys(other).some(variant => restyledVariants.includes(variant)), | ||
'Deprecation Warning: Material-UI: Your are passing a variant to createTypography ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"You are"
warning( | ||
useNextVariants || !Object.keys(other).some(variant => restyledVariants.includes(variant)), | ||
'Deprecation Warning: Material-UI: Your are passing a variant to createTypography ' + | ||
'that will be restyled in the next major release without indicating that you ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"release, without"
That sounds simmilar to the Volkswagen approach. 😄 Jokes aside, the deprecation looks good. |
5db2370
to
a4b65b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great first iteration! :) I'm adding this pull-request to the v3.2.0 milestone, we need a minor upgrade to release the new deprecation warnings. We can't do it in a patch.
.size-limit.js
Outdated
@@ -16,13 +16,13 @@ module.exports = [ | |||
name: 'The initial cost people pay for using one component', | |||
webpack: true, | |||
path: 'packages/material-ui/build/Paper/index.js', | |||
limit: '17.5 KB', | |||
limit: '18.8 KB', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow
@@ -13,6 +13,7 @@ export interface TypographyProps | |||
headlineMapping?: { [type in Style]: string }; | |||
noWrap?: boolean; | |||
paragraph?: boolean; | |||
useNextVariants?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should only have this option in the theme. What's the use case for having it in the property? Progressive migration? I'm fine dropping that for the simplicity we get in exchange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably a good idea. Reduces added code (and potential risk for bugs). I wouldn't recommend mixing different typography in the first place. It's hopefully a niche use case and can always be solved by using different themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial use case was for the Typography
test suite were I didn't want to wrap every Typography
with a MuiThemeProvider
so that it doesn't throw deprecation warnings. A few iterations later this can be achieved by just passing a theme with ignoreDeprecationWarnings
. It's a little hacky but given that this is only existing in our codebase for a limited period of time I'm ok with a brittle test suite.
@@ -100,7 +100,7 @@ function AppDrawer(props, context) { | |||
<div className={classes.toolbarIe11}> | |||
<div className={classes.toolbar}> | |||
<Link className={classes.title} href="/" onClick={onClose}> | |||
<Typography variant="title" color="inherit"> | |||
<Typography variant="headline6" color="inherit"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of brevity, I'm wondering if we shouldn't be using "h" instead of "headline". It's something people are already used to. Here it would be variant="h6"
. It's an open suggestion, I have no convinctions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither. I was looking at the material-web-components implementation and they use headlineX
. Using hX
would match the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mui-org/core-contributors What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for h6
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like that we abstract away the actual html markup. It looks more consistent if we write out the meaning for every variant and not just a select few. But in this instance I don't really care how we go about naming these. I'm interested in what twitter has to say about it though. Did you already tweet this out @oliviertassinari?
|
||
return ( | ||
<div className={classes.root}> | ||
<Typography variant="headline1" gutterBottom suppressDeprecationWarnings> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suppressDeprecationWarnings
is a dead property right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, will fix that and look if I forget anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That hole section was conceptually wrong. I wanted to display deprecated types but failed to do so in any way. It is now displaying deprecated and restyled variants with a v1 typography theme.
assert.fail('got no error', `expected a warning to match '${expectedWarning}'`); | ||
} | ||
} catch (e) { | ||
assert.isTrue(expectDeprecation, 'expected no deprecation but got a warning anyway'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one occurrence of assert.isTrue
in the code base (one that you added). What do you think of using strictEqual(x, true)
? At least it's minimizing the entropy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned that you should always use the appropriate matcher methods for readability. I don't see why I should not use the matchers that are available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's a tradeoff between having meaningful error messages vs controlling your assertion. assert.strictEqual
leaves no room for doubt, I think that it's very important, it's a brainless ===
.
For instance, Is isTrue
using strict or nonstrict equal?
The exceptions are .deepEqual(
, .throw(
and .include(
because using strict equal their requires too much boilerplate or yield a very poor debug message when it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Named matchers (with is
or be
) use strict equality in chai, jest and jasmine. Those are the most popular assertion libraries in js. I don't agree that there is room for doubt if we are talking about code in a unit test.
I'm not arguing about this particular line of code but I think there is a reason assertion libraries even include noop chain methods to further improve readability in unit tests.
I think my main issue is chai's assert library since I'm used to jest which is much better to read and the discussion whether to use toBe(true)
or toBeTrue()
would be fruitless (funny enough jest doesn't have toBeTrue
but toBe[Null|Undefined]
).
I'll change it for consistency.
/* eslint-disable key-spacing, no-multi-spaces */ | ||
// prettier-ignore | ||
const nextVariants = { | ||
headline1: propertiesForCategory(fontWeightLight, 96, caseSentence, -1.5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their negative letter-spacing is causing issues. For instance when using different fonts. I have been minimizing the value as much as possible. Also, why not using the em
value directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to follow the spec as closely as possible and they use other values that are then converted depending on the system (web, ios, android). material-web-components uses the same strategy. I would keep the conversion so that we can easily spot differences if something changes or a if a value is wrong.
I mean any property-value combination can cause problems depending on the font-family, no? The letter-spacing is also not negative on my setup. Rember that those values are also converted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if somebody is customizing typography by using another font-family then they are also responsible for adjusting letter-spacing if this causes problems. Maybe we can let users pass a custom function to convert letter-spacing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the letter-spacing was designed for Roboto we could not apply the letter-spacing if another font-family is applied. Does this sound like a good compromise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the letter-spacing was designed for Roboto we could not apply the letter-spacing if another font-family is applied. Does this sound like a good compromise?
It sounds perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way the old typography variants were using letter-spacing.
// prettier-ignore | ||
const nextVariants = { | ||
headline1: propertiesForCategory(fontWeightLight, 96, caseSentence, -1.5), | ||
headline2: propertiesForCategory(fontWeightLight, 60, caseSentence, -0.5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having caseSentence
as a default behavior is opinionated. I think that we will be better off not having it by default. Very few, to no CSS libraries comes with this style as a default. I don't think that people expect this behavior. Looking at the source code of our application at work, we have only one use case for '&:first-letter': { textTransform: 'uppercase' },
while many for disabling it. So, what do you think of removing this default behavior and adding a property to enable it? (But I'm not even sure we need a property for that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification defines it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already something that can be disabled with:
createTheme({
typography: {
caseSentence: {}
}
})
Maybe add a documentation for the createTypography
part of createTheme
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yet, MCW isn't implementing it either. I think that we should remove this default behavior, defer the choice to the users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case made. Use inherit
instead or just leave it blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by using inherit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text-transform: inherit
mwc is doing the same. They are doing the same for text-decoration. Not sure if/why we should follow along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default text transformation doesn't inherit. I'm wondering what's their motivation for it.
In the doubt I would stick the the browser default behavior, à la Bootstrap: https://github.com/twbs/bootstrap/blob/b652932f0cdfa3ca054f9b607d56f3bdb046d284/scss/_type.scss.
@oliviertassinari Thank you for the feedback. That is definitely a minor bump because of new variants and deprecation warnings. |
I realized that I'm sometimes using |
@eps1lon React is using "suppress" over "ignore" for their API but you will find more occurrences of "ignore" in their codebase. |
@oliviertassinari I went with |
ab46bd3
to
89c5a06
Compare
bd76504
to
4edc8ac
Compare
9edc157
to
cbb887a
Compare
cbb887a
to
cb8d79a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not squash my work while PRs are still in review.
Does the squashed commit contain work of yours? @oliviertassinari
@@ -59,7 +59,7 @@ function ListItemText(props, context) { | |||
if (primary != null && primary.type !== Typography && !disableTypography) { | |||
primary = ( | |||
<Typography | |||
variant="subheading" | |||
variant="subtitle1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change:
- regression-List/AvatarListItem.png
- regression-List/PrimaryActionCheckboxListItem.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We miss the line-height. I'm gonna add that and see if we can keep subtitle1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a different variant name is already breaking.
subheading
uses a different component, people could override the style etc. There is no reason to change that at the moment. We wanted to make this a minor change only and now it's a major one (concerning semver).
@@ -9,7 +9,7 @@ import ListItem from '../ListItem'; | |||
export const styles = theme => ({ | |||
/* Styles applied to the root element. */ | |||
root: { | |||
...theme.typography.subheading, | |||
...theme.typography.subtitle1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change:
- regression-Menu/SimpleMenuList.png
body2Next: propertiesForVariant(fontWeightRegular, 14, 0.25), | ||
buttonNext: propertiesForVariant(fontWeightMedium, 14, 0.75, caseAllCaps), | ||
captionNext: propertiesForVariant(fontWeightRegular, 12, 0.4), | ||
overline: propertiesForVariant(fontWeightRegular, 10, 1.5, caseAllCaps), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's was deliberately formatted that way to match the typography scale table to easily spot mistakes. Either revert or remove eslint-disable and prettier-ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch, I'm gonna remove
/* eslint-disable key-spacing, no-multi-spaces */
// prettier-ignore
@eps1lon No, I haven't changed your work (at the exception of the conflicts I have resolved), I have done the squash yesterday. It was the only way I have found to rebase the pull-request on master. I have started the second iteration this evening. I think that it's the merge from master into the PR that cause the rebase issue. |
You can always fall back to merge for long-living branches. I had to do the same. It's all good since I can save a copy of the old branch. It's just that I went through quite a bit of iterations and I wanted to keep the commit messages for a bit. |
3309e15
to
d4b065f
Compare
I have used https://github.com/material-components/material-components-web/blob/2cf84876fec45a0c8d31eff2ec47d48b11c8fdac/packages/mdc-typography/_variables.scss as the baseline for the line height values. |
assert.strictEqual(typeof createMuiTheme, 'function', 'should be a function'); | ||
assert.ok(muiTheme.palette, 'should have a palette'); | ||
assert.strictEqual(typeof createMuiTheme, 'function'); | ||
assert.strictEqual(typeof muiTheme.palette, 'object'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly the reason you use named matchers. Adding your own implementation reduces readability and can introduce bugs which you did. typeof null === 'object'
while assert.ok
would've caught this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use assert.strictEqual(muiTheme.palette != null, true);
but I'm more interested in knowing the palette is an object instead of a boolean / string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings should be displayed by default. They can already be disabled via theme
configuration and environment variable.
@@ -57,6 +57,7 @@ function CardHeader(props) { | |||
title = ( | |||
<Typography | |||
variant={avatar ? 'body2' : 'headline'} | |||
suppressDeprecationWarnings | |||
className={classes.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation warnings are a hint for the user so that they know where a breaking change will happen in the next major release.
A breaking change will happen in that component. The user will not be warned about this. This is bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a deprecation warnings just noise for the users when it's not actionable? theme.suppressDeprecationWarnings
is an escape hatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actionable since it will be a breaking change in the next major relase. See https://semver.org/#how-should-i-handle-deprecating-functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not part of the public API, is it? It's not something users are doing wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari They are by not switching to a typography v2 theme if they're using this component. It's all about where the behavior of software will change. Users can't deal with per instance because we decided against that.
@@ -73,7 +74,7 @@ function ListItemText(props, context) { | |||
if (secondary != null && secondary.type !== Typography && !disableTypography) { | |||
secondary = ( | |||
<Typography | |||
variant="body1" | |||
variant="body2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change.
}, | ||
noWrap: false, | ||
paragraph: false, | ||
suppressDeprecationWarnings: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the "partial migration strategy" discussion all over again. We decided against that and now we reintroduce it by allowing partial suppression of deprecation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative of not introducing this property is to upgrade the variant usage in our core components. You have made a good point on raising it's a breaking change and should be deferred to v4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is to leave it as is and let the user decide how to treat the warning. We don't have to update internal usage now. But the user should know where he has to expect changes with the next major release. That is the hole point of deprecation warnings in a minor before a major release.
Updating variants in our internal components is scheduled for the next major release in a separate PR as is described in #12741
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the user should know where he has to expect changes with the next major release.
What's the value in that? I don't understand the incentive. You throw/warn so people upgrade their code, they can't do anything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can switch to the Typography v2 theme which we discussed is the preferred way. A global, immediate switch.
0091c2f
to
52da9f4
Compare
@oliviertassinari So I revisited the The prop offered a way to ignore the warnings on a per component base with which I do not agree because we also decided against a prop to switch to v2 on a per component base. The actual problem this prop tried to solve was that some components used deprecated variants internally. Using a v2 theme would already map those to v2 variants so there would be no breaking change for users with a v2 theme when switching to the next major release but they would still get warnings. So what I propose is that we consider this prop for internal usage only (annotate it to be excluded from docs and don't add type declarations). Users should only have two strategies:
Furthermore rename
|
@eps1lon It's perfect. Let me try to rebase that. |
try-catch also catches assert errors which makes it much harder to reason about.
b43f0fd
to
ae6ef93
Compare
Boom, well done! |
* [core] Implement new material typography spec * rename headline in h * [Typography] Simplified test suite try-catch also catches assert errors which makes it much harder to reason about. * let's merge :)
For future reference, this type of deprecation warning really needs a rate limit or deduplication. This is producing so many messages that they pretty much have to be suppressed or else other (more important) messages will get lost. |
@jimrandomh I was wondering about it this afternoon. I can image. |
Putting I have pinned our project on the previous release for now, since this is happening at the same time as we're upgrading another library which is producing lots of real errors that we need to pay attention to without noise mixed in. |
This was already fixed in HEAD.
We did the same thing at work. We will release v3.2.1 this weekend, hopefully, it will be fine. |
@eps1lon Should have we done: const theme = createMuiTheme({
suppressDeprecationWarnings: true,
}); instead of? const theme = createMuiTheme({
typography: {
suppressDeprecationWarnings: true,
},
}); But I'm in favor of:
|
and partialmigration strategy(partial only applicable toTypography
)Closes #11667
Closes #12377
MUI_SUPPRESS_DEPRECATION_WARNINGS
to a truthy value will ignore all warnings. This is useful if warnings break your unit testscreateMuiTheme({ typography: { suppressDeprecationWarnings: true } })
and using this in aMuiThemeProvider
will cause no deprecation warnings to be logged.createMuiTheme({ typography: { useNextVariants: true } })
and using this in aMuiThemeProvider
will enable typography v2 across the board. It will still log warnings for deprecated variants but not restyled variants.Deprecation
To learn more about the upgrade path, follow https://material-ui.com/style/typography/#migration-to-typography-v2.