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

[Typography] Small improvements #13129

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 6, 2018

  • Switch the default variant between typography v1 and v2.
  • Don't apply the scale twice
  • Round the letter spacing value
  • Makes Material-UI less opinionated regarding the letter-spacing value. I think that the specification is promoting a too strong letter-spacing value, at least for a minor release. I'm softening the change.

@oliviertassinari oliviertassinari added the component: Typography The React component. label Oct 6, 2018
@oliviertassinari oliviertassinari force-pushed the fix-typography-issues branch 2 times, most recently from e5a409f to ed71e84 Compare October 6, 2018 15:03
@oliviertassinari oliviertassinari merged commit 253ee26 into mui:master Oct 6, 2018
@oliviertassinari oliviertassinari deleted the fix-typography-issues branch October 6, 2018 16:08
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I'm really lost how it is less opinionated to apply letter-spacing you seem appropriate instead of the letter spacing that is defined in the spec. I also fail to see why this is a concern for versioning. We're adding a new feature that is completely backwards compatible. What do you mean by "softening"?

It's the second time you want to change this. Do we want to implement the spec or do we interpret the spec?

@@ -324,7 +332,6 @@ Typography.defaultProps = {
headlineMapping: defaultHeadlineMapping,
noWrap: false,
paragraph: false,
variant: 'body1',
Copy link
Member

Choose a reason for hiding this comment

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

That's a breaking change. What's the purpose of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How is this a breaking change?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 6, 2018

do we interpret the spec?

We do interpret the specification, as Google is doing with all its products. Even the Material Design specification website and the MDC Web implementations are interpreting the typography spec. There are two topics here.
The first one is around keeping 14px as the default font size when using the Typography component. To achieve that, we need to switch body 1 with body 2. Also, I think that we should use the next typography for the regression tests.
The second topic is around not changing the letter spacing too quickly. We can always change the letter spacing in v4.0.0 to a final value. I don't think that we should be updating our implementation faster than Google is updating its products. I haven't seen them executing on the new letter spacing yet. One step at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants