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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tabs] Awkward line height when 2 lines #14538

Closed
2 tasks done
Studio384 opened this issue Feb 15, 2019 · 9 comments
Closed
2 tasks done

[Tabs] Awkward line height when 2 lines #14538

Studio384 opened this issue Feb 15, 2019 · 9 comments
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@Studio384
Copy link
Contributor

Studio384 commented Feb 15, 2019

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 馃

a989bab3-e062-4dcf-a679-0afd21b9b4e6

It should look like this:

annotation 2019-02-15 110003

Current Behavior 馃槸

When a tab title takes multiple lines, the line height of 1.75 is to large.

588df98b-6a19-427c-9b54-56c2ab9b0401

Steps to Reproduce 馃暪

Link:

  1. https://material.io/design/components/tabs.html#anatomy
Tech Version
Material-UI v3.9.2
@eps1lon
Copy link
Member

eps1lon commented Feb 15, 2019

Looks like this is intended for our components: https://material-ui.com/demos/tabs/#wrapped-labels. It probably changed in the spec.

Some quick hacking with white-space: nowrap; text-overflow: ellipsis; didn't do the trick but something along the lines should be possible.

@Studio384
Copy link
Contributor Author

My point isn't that it wraps, that is the recommended way to do so in the specs, the problem is that the line height is to high and pushes the text to the top and bottom of a tab which look awkward. The space between the top and bottom line of text should be reduced.

@eps1lon
Copy link
Member

eps1lon commented Feb 15, 2019

In the spec it looks like the font size is changed for wrapped labels. Can we do the same thing too dynamically?

The space between the top and bottom line of text should be reduced.

What dimensions should it have and why?

@eps1lon eps1lon added design: material This is about Material Design, please involve a visual or UX designer in the process component: tabs This is the name of the generic UI component, not the React module! labels Feb 15, 2019
@Studio384
Copy link
Contributor Author

Studio384 commented Feb 15, 2019

Actually, the specs seem to discourage such behavior:

Don鈥檛 resize text labels to fit them onto a single line. If labels are too long, wrap text to a second line or use scrollable tabs.

And that's not what this issue is about.

Anyways, instead of this:

588df98b-6a19-427c-9b54-56c2ab9b0401

It should look like this:

annotation 2019-02-15 110003

...just like the specs show: no way to large lineHeight for the text in tabs. It should be reduced from the current 1.75 to just 1 or 1.25 (I didn't look into how high it is in the specs).

As to why: just to follow the specs and because now a 2 line tab messes with the height of the tabs component while that just isn't necessary.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 15, 2019

@Studio384 The logic was removed in #13969. I would vote for removing the deprecated logic. I think that we should let our users handle the problem with a new property, manually. We have tried to handle the problem automatically but we can't provide a good implementation without significantly increasing the bundle size. What do you think about this strategy?

@Studio384
Copy link
Contributor Author

Studio384 commented Feb 15, 2019

This is not about the font size. It's about the way to large line height in tabs.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 15, 2019

This is not about the font size.

The line height is proportional to the font size.

@oliviertassinari oliviertassinari added new feature New feature or request breaking change good first issue Great for first contributions. Enable to learn the contribution process. labels Feb 15, 2019
@eps1lon
Copy link
Member

eps1lon commented Feb 15, 2019

just like the specs show

I can't easily spot 2 or 4px differences. It would've been enough to say that the "tab height changes when lines are wrapped. This does not follow the spec for single lines."

However the spec doesn't mention how the dimensions should be for two line tabs. It doesn't even specify what typography variant to use so we can only infer that. line-height should be proportional to font-size according to spec which goes against a fixed tab height. We need to reconcile these two facts.

@tomasztunik
Copy link

Agreed with this - if you look at https://material.io/develop/web/components/tabs/tab-bar/ the span holding the label content has line-height: 1 in spite of the higher, relative line-height on the button. Seems as if the css implementation on the label and wrapper is bit off from the reference implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants