Skip to content
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

[system] Add typography prop that will pull from theme.typography #23451

Merged
merged 7 commits into from
Nov 11, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Nov 9, 2020

Note: It's best to hide whitespace changes when reviewing - https://github.com/mui-org/material-ui/pull/23451/files?diff=split&w=1

This PR adds support for font prop as part of the system's typography that will pull the values defined in the theme.typography. It should simplify scenarios for pulling all values defined for a typography variant. For example:

-   const round = (value) => Math.round(value * 1e5) / 1e5;
// ...
<Box
  sx={{
-  fontFamily: 'fontFamily',
-  fontSize: "body1.fontSize",
-  fontWeight: 'body1.fontWeight',
-  letterSpacing: `${round(0.15 / 16)}em`,
-  lineHeight: 1.5,
+  typography: 'body1',
 }}
>
  @material-ui/system body1
</Box>

Fixes #23190
Fixes #17504

@mui-pr-bot
Copy link

mui-pr-bot commented Nov 9, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 06cdf07

@mnajdova mnajdova added the package: system Specific to @mui/system label Nov 9, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 9, 2020

Are we should we want to overload the shorthand font CSS property? This would mean that sx is no longer a CSS superset.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we should we want to overload the shorthand font CSS property? This would mean that sx is no longer a CSS superset.

@eps1lon From what I understand, it's similar to padding: 1. If the font has a value that matches the theme, then it gets precedence over the actual value. So both font: 'body1' and font: '1.2em "Fira Sans", sans-serif;' works.

@oliviertassinari oliviertassinari added the new feature New feature or request label Nov 9, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 9, 2020

So it's not actually a superset. We should stop calling it a superset then.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 9, 2020

@eps1lon Do you have an example of a value that is supported by CSS that isn't by the system? (a counter example that proves that it's not a superset)

@mnajdova
Copy link
Member Author

mnajdova commented Nov 9, 2020

If there is a concern with font as it is a CSS property we could use typography as key instead. But, as the prop can behave as css property if the value is css value, I don’t see a big problem to be honest.

@eps1lon
Copy link
Member

eps1lon commented Nov 10, 2020

Do you have an example of a value that is supported by CSS that isn't by the system?

font: caption; works in CSS but different in sx. In a superset it would work the same. Or it wouldn't work in CSS but work in sx.

"superset" is a well-known and defined term which helps understand a particular issue by putting constraints in place. We shouldn't use that term with a custom interpretation. Otherwise it has an adverse effect on learning sx: It requires knowing what Material-UI means by "superset".

@mnajdova
Copy link
Member Author

font: caption; works in CSS but different in sx. In a superset it would work the same. Or it wouldn't work in CSS but work in sx.

This is a very good corner case, thanks for pointing it out. I believe we can change this prop then to typography so that we can avoid these collisions.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 10, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 10, 2020
@mnajdova mnajdova changed the title [system] Add font prop that will pull from theme.typography [system] Add typography prop that will pull from theme.typography Nov 10, 2020
@mnajdova mnajdova requested a review from eps1lon November 10, 2020 16:58
@mnajdova mnajdova merged commit e58fc20 into mui:next Nov 11, 2020
@oliviertassinari
Copy link
Member

Do we miss a demo for https://next.material-ui.com/system/typography/?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system
Projects
None yet
4 participants