-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[material-next] Update Material You typescale tokens #39514
[material-next] Update Material You typescale tokens #39514
Conversation
Netlify deploy previewhttps://deploy-preview-39514--material-ui.netlify.app/ @mui/material-next: parsed: +0.10% , gzip: +0.06% Bundle size reportDetails of bundle changes (Toolpad) |
}, | ||
medium: { | ||
family: 'Roboto', | ||
weight: '500', | ||
lineHeight: 16, | ||
// TODO: label.medium has an alternative font weight https://m3.material.io/styles/typography/type-scale-tokens |
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 sure if it's better to have a weightProminent
property or add a label.mediumProminent
variant but we can worry about it later, referencing material-web.dev this is very rarely used: https://github.com/search?q=repo%3Amaterial-components%2Fmaterial-web%20prominent&type=code
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 sure if it's better to have a weightProminent property or add a label.mediumProminent
I think adding weightProminent
is more in line with how material names their tokens.
we can worry about it later
I agree
size: 12, | ||
tracking: 0.5, | ||
lineHeight: 16, |
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 can be misleading for people using the theme to divide this value with the size to get the specific CSS line height value. What I mean is that this will be used in most cases like:
line-height: calc(theme.sys.typescale.body.small.lineHeight / theme.sys.typescale.body.small.size)
Should we specify in the name that it is a px value? If it was up to me I would define two values lineHeightPx
that would be 16 in this case and lineHeight
(the actual value that can be used in the lineHeight CSS prop - note this needs to be calculated based on the theme values :)).
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 will let @DiegoAndai decide. It may be useful to have a design perspective, cc @danilo-leal
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.
If it was up to me I would define two values
lineHeightPx
that would be 16 in this case andlineHeight
This makes perfect sense to me. I think it will avoid confusion as users would expect the lineHeight
to be the unit-less CSS value, as that's the most common.
Also, adding the lineHeight = lineHeightPx / size
in the theme will save us from doing line-height: calc(theme.sys.typescale.body.small.lineHeight / theme.sys.typescale.body.small.size)
on each component, which we're doing right now 😅.
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.
@mnajdova @DiegoAndai Good idea ~ I've added a task in #38804 (let's make this change separately)
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.
@mnajdova @DiegoAndai Good idea ~ I've added a task in #38804 (let's make this change separately)
Sounds good too. My point was, if we plan to rename it, why do a breaking change for just one release, but it's experimental package so it should be fine, it shouldn't affect lots of people.
This PR adds some missing tokens from the material-next typescale.
While working on TextField (#39503, #39483) I noticed we haven't included the latest tokens yet