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

Add typography styling attributes to Typography component #17614

Closed
lcswillems opened this issue Sep 29, 2019 · 8 comments
Closed

Add typography styling attributes to Typography component #17614

lcswillems opened this issue Sep 29, 2019 · 8 comments
Labels
duplicate This issue or pull request already exists

Comments

@lcswillems
Copy link
Contributor

  • [ x ] I have searched the issues of this repository and believe that this is not a duplicate.

Motivation 🔦

Typography styling is currently painful: changing font-size, font-weight, font-style, etc...

For example, if I want to make a H1 title bold, I need to write:

<Typography component="h1"><Box fontWeight="fontWeightBold">My title</Box></Typography>

Whereas it would be much more natural and simple if I could do:

<Typography component="h1" fontWeight="fontWeightBold">My title</Typography>

Summary 💡

I propose to add the textAlign, fontWeight, fontSize, fontStyle, fontFamily, letterSpacing, lineHeight attributes of the Box component to the Typography one.

A proof that the Typography component lacks these attributes is that on your typography system page, you only use Box component. It doesn't make sense for me.

Anyway, thank you for all your great work!!

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

A proof that the Typography component lacks these attributes is that on your typography system page, you only use Box component. It doesn't make sense for me.

This page is about typography of the design system not the Typography component.

I would rather let the Box handle this since it's purpose is to be your design system primitive.

The Typography component is specifically aimed at Typography in the Material design system. It should stay a very dumb and simple wrapper IMO. Specifically
<Typography component="h1" fontWeight="fontWeightBold">My title</Typography> is something we likely don't want to support. Why is this level one heading bold all of the sudden? Putting extra emphasis on a level one heading seems redundant. You probably want to style every level one heading with a bold font weight making it obvious you're not using the Material design system anymore.

@eps1lon eps1lon added component: Typography The React component. new feature New feature or request labels Sep 29, 2019
@lcswillems
Copy link
Contributor Author

lcswillems commented Sep 29, 2019

Thank you for your fast answer!

I don't see why I should put all my style on a Box component when I don't need a Box component and I also don't see why the Typography should be dump.

On my website, there is one particular h1 that I want it to be bold. This is only one example I gave you. But, it may be possible that I need to italicize only one particular paragraph or I need to change the alignment of one particular paragraph (make it right-aligned or center), and it would be more natural if I don't need to add an extra Box inside the Typography component.

@lcswillems
Copy link
Contributor Author

If you don't want to add a fontWeight attribute to Typography why do you add a maxWidth property to Container? This is the same kind of thing.

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

If you don't want to add a fontWeight attribute to Typography why do you add a maxWidth property to Container? This is the same kind of thing.

maxWidth on the Container is not similar to max-width in CSS.

These use cases seem to be very specific. I'd like to hear a second opinion from another core member. Otherwise we'll see how many upvotes this issue gets since this is currently possible to achieve for library consumers. Nothing in the core library is preventing you from giving a particular heading a different font-weight as far as I can tell.

@lcswillems
Copy link
Contributor Author

lcswillems commented Sep 29, 2019

Thank you for your answer.

Nothing in the core library is preventing you from giving a particular heading a different font-weight as far as I can tell.

No, it doesn't prevent me of course, but it would be nicer from my point of view.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2019

This is already discussed in #15561.

@oliviertassinari oliviertassinari added duplicate This issue or pull request already exists and removed component: Typography The React component. new feature New feature or request labels Sep 30, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2019

The Typography component is specifically aimed at Typography in the Material design system. It should stay a very dumb and simple wrapper IMO. Specifically
My title is something we likely don't want to support. Why is this level one heading bold all of the sudden? Putting extra emphasis on a level one heading seems redundant. You probably want to style every level one heading with a bold font weight making it obvious you're not using the Material design system anymore.

@eps1lon I don't think that Material Design is our main concern in this context. From my perspective, the Typography component is interesting in the sense that it can pull the theme.typography variants out (h1, body1, etc.). While these variants enforce consistency and should be enough most of the time. It's not always the case. I think that it would be interesting to allow access to all the system props, in all the core components.

Now, I believe the concern is identical to #15561, we can continue the discussion there.

@lcswillems
Copy link
Contributor Author

You are right, it is the same discussion. Thank you for your response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants