Material-ui: Typography `noWrap` hides high(vertical) characters

[albinekb]

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

Expected Behavior

When I use noWrap on Typography, it should not cut my "g" off, just like without noWrap

Current Behavior

Steps to Reproduce (for bugs)

Use https://material-ui-1dab0.firebaseapp.com/demos/drawers/#responsive-drawer and add a g
in the title

Context

This is happening because adding noWrap adds overflow: hidden, is this needed? Can we switch to overflow-x: hidden or remove it completely?

Another fix would be to make the component with overflow: hidden fit higher chars.

Thoughts?

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | latest |
| React | 16.0.0 |
| browser | chrome canary|

[oliviertassinari]

Can we switch to overflow-x: hidden or remove it completely?

I can't reproduce the issue, but using overflow-x: hidden sounds like a good idea (we need to keep it)

[JeromeFitz]

I can swap this to overflow-x: hidden. 👍️

[JeromeFitz]

That broke via argos however adding (while keeping overflow: 'hidden':

• lineHeight: 'inherit'

Has it showing appropriately.

Before (as mentioned by @albinekb):

After:

However, it does move up the main content a tinch.

I am going to add this to the PR to see how argos handles this (since I cannot duplicate the scrollbars).

[JeromeFitz]

The tinch up breaks argos (as it should). I will try a few more combos and see if we can get desired behavior while keeping existing.

[JeromeFitz]

When we add overflowX: 'hidden' it looks to set overflowY: 'auto'. So without a hack, that will not work (this looks to be per CSS3 Spec).

When we put lineHeight: 'inherit' on the core Typography noWrap element, this works with keeping overflow: 'hidden' while having no scrollbars, the desired ... effect, and no cut-offs on -g for Title.

<Typography type="title" color="inherit" noWrap>

However, in the examples for:

• MiniDrawer
• PermanentDrawer
• PersistentDrawer
• ResponsiveDrawer

In their content, they are also using:

<Typography type="body1" noWrap>

Which is causing the display shift in argos. Since in these examples the height, marginTop is handled in the demos themselves.

Note: AppFrame is using the Typography Component with noWrap with its type of title - this looks to be the only other place.

So we could:

1. Keep noWrap on content in the demos: Change the classes.content to adjust the height, marginTop in its root and for [theme.breakpoints.up('sm')].

2. This would keep argos passing without much trouble.

3. Remove noWrap on content in the examples.

4. This would bring the text back up to its expected height (via argos) but break its eventual end-state due to the new line (causing a new run of expectations in argos).

5. Allow for new adjusted behavior in argos in how noWrap works going forward with only a change to Typography.noWrap.

6. Go back to the drawing board. 🤣️

I can make the changes to move forward however you see fit.

Here are some screenshots:

Current:

Result of 1 before margin adjustment:

Result of 1 after margin adjustment:

Result of 2 with noWrap removed:

[JeromeFitz]

Went with Option 1 in the above comment to see if we could use argos in its current style regression state.

Bumped up the content container pixels by 2 where appropriate for the drawers and everything is passing the latest PR. 🖼️

[oliviertassinari]

@JeromeFitz Thanks for pushing it forward!

After looking more into the issue, I confirm it. Maybe we shouldn't be using line-height: 1 in the first place. This StackOverflow answer is interesting. As we can see in the CSS spec:

The font size corresponds to the em square, a concept used in typography. Note that certain glyphs may bleed outside their em squares.

material-components-web is using some different values. Still, their display 4 and 3 are affected by the same issue.

So, I would suggest using line-height: 1.11 instead of line-height: 1 as the minimal line-height value.

But then adding the following style background: transparent url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAAICAYAAADTLS5CAAAAFklEQVQYV2NkgAJG8hjl5eX/Ozs7GQEVHAQJMG4X6QAAAABJRU5ErkJggg==); } show that we have some Vertical Rhythm issue:

It's something we can compare against the spec:

(they are using a Print design Baseline grid as opposed to the Web one).
Here is a great article on the Vertical Rhythm topic.

[JeromeFitz]

@oliviertassinari Thank you as always for such detail. This is incredibly helpful not just from this issue level, but the entirety of your project scope.

The Vertical Rhythm topic is a great read. I've been making some adjustments to a local branch with the comments you shared (lineHeight, material-web-components, etc.) , however, before making any PR's going forward I will come back here with questions.

Though, as I was writing, I did just see you self-assigned this. I can stand down as well if the extra cook in the kitchen is not needed and would be more work on your end. Thanks again!

[JeromeFitz]

@albinekb, a (possible) short-term solution on your end would be to add the lineHeight directly to your element for now. You may need to tweak to fit your application:

<Typography type="title" color="inherit" style={{ lineHeight: 'inherit' }} noWrap>
  Inkommande bokningar
</Typography>

[albinekb]

Thanks, that's totally acceptable for me 👍  @JeromeFitz
And just WOW to the amount of effort and knowledge everyone put into this library, it's just amazing. I love it, it's so easy and straightforward to use. Which makes it a lot of fun 👍

[JeromeFitz]

Awesome!

However, all the credit goes to @oliviertassinari ❤️ 😀️ 👍️