[core] Fix MenuItem text clipping #2265

reiv · 2018-03-17T00:20:53Z

Fixes #2177

Reviewers should focus on:

Does this break anything that uses MenuItem?

Screenshot

Edit: The font shown here, Segoe UI (Windows default), seems to have a weird baseline, causing it to appear slightly off-center compared to other fonts. This isn't fixable without replacing the font altogether.

reiv · 2018-03-19T03:30:52Z

Merge branch 'develop' into rv/fix-menuitem-clipping

Preview: documentation | landing | table

giladgray · 2018-03-19T19:21:55Z

this causes menu items to render at 32px height by default, which is going to be very obnoxious to fix. all the padding logic assumes 16px height of icons and text.

llorca · 2018-03-19T20:33:39Z

yeah, also breaks the grid. @reiv any thoughts on how to preserve 30px height for menu items here?

reiv · 2018-03-19T22:44:01Z

Are menu items intended to be single-line? I'm asking because the pt-menu-item style is also applied to the nav links in the documentation, which wrap on longer lines:

For single line, the obvious solution is to set an absolute height of 30px. Otherwise, we could remove the vertical padding and set line-height to 30px, but that causes multi-line menu items to have too much spacing between lines:

@llorca Not sure which grid you are referring to here. Is it a design requirement that the height of menu items be a multiple of $pt-grid-size?

llorca · 2018-03-19T22:49:41Z

yes, it's gotta be a multiple of $pt-grid-size ($pt-grid-size * 3.2 doesn't count)

IIRC, by default they're one-liners, but it should be able to override that behavior to get a multiline item with correct line-height

llorca · 2018-03-19T22:51:22Z

removing .pt-text-overflow-ellipsis from the text within .pt-menu-item makes it nice and multiline, can be turned on and off with the multiline prop

reiv · 2018-03-19T22:55:06Z

Oh, so if there's an explicit multiline prop, then couldn't we just style the menu item based on that? For single line, set height: 30px or line-height: 30px and remove vertical padding. For multiline, well, don't. Would that work?

llorca · 2018-03-19T22:57:40Z

I think that could work yes, it's a 2.0 thing

This reverts commit d7210dd.

reiv · 2018-03-19T23:34:11Z

Add CSS class for single-line menu items

Preview: documentation | landing | table

llorca · 2018-03-19T23:37:27Z

now the text is down 1px :( looks vertically misaligned. we're going to need at least an 18px line-height then I suppose?

also the multiline mode looks broken?

reiv · 2018-03-19T23:48:10Z

@llorca That's weird, considering the change shouldn't affect multiline menu items whatsoever (it only adds a class when multiline == false). What did you do so I can reproduce?

llorca · 2018-03-19T23:50:35Z

I was just turning pt-text-overflow-ellipsis on and off in the inspector

reiv · 2018-03-19T23:53:20Z

No, you need to use React dev tools to explicitly toggle multiline (or remove the pt-menu-item-single-line class from the outer <a> element as well).

Should look like this:

Edit: looks like the label icon is off-center in single line mode as a result of line-height: normal.

reiv · 2018-03-20T00:19:23Z

Apply line-height directly to menu item label

Preview: documentation | landing | table

llorca · 2018-03-20T00:43:43Z

packages/core/src/components/menu/_common.scss

@@ -45,6 +45,15 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa
    color: $pt-text-color-disabled;
  }

+  &.pt-menu-item-single-line {


😢 sad we need an extra class just for that

Alternatively we could define a class for multiline instead. Would that be preferable?

llorca · 2018-03-20T00:44:08Z

packages/core/src/components/menu/_common.scss

+    // to be clipped. Instead, we set the height directly to adhere to the
+    // grid.
+    height: $pt-grid-size * 3;
+    line-height: normal;


normal makes the text misaligned, vertically not centered by 1px. unset seems to work, although I have no idea why?

I can't see the misalignment on my end, so I'm assuming this is font-specific. Can you post a screenshot please?

The misalignment appears in your own screenshot:

Notice how "Settings..." is 1px down

Hm, I thought that was due to Segoe UI's baseline. Here it is with sans-serif (Arial):

What's odd to me is that changing it from normal to unset has no effect on Firefox*, but on Chrome it does actually fix the misalignment with Segoe UI.

* That is, the computed line-height changes from 20px (normal) to 18px (unset), but visually it's all the same for some reason.

Another problem with unset: Firefox, zoom level 140%, clipping still occurs:

Had to look up the difference between normal and unset:

normal uses the browser default, whereas unset inherits the nearest parent's value. In this case it would be <body>, which gets its line-height from here:

blueprint/packages/core/src/common/_variables.scss

Lines 30 to 31 in b4de4e6

// a little bit extra to ensure the height comes out to just over 18px (and browser rounds to 18px)

$pt-line-height: ($pt-grid-size * 1.8) / $pt-font-size + 0.0001 !default;

So unset gives us an 18px line height. Furthermore, it seems that in Chrome, odd pixel values for line-height shift everything down by 1px, as shown here:

This affects most fonts, Arial being one of the exceptions. I'm guessing normal ends up producing an odd pixel value (inspector doesn't show the actual value), causing the misalignment.

llorca · 2018-03-20T00:45:37Z

@reiv I assume the original clipping was due to the overflow, have we considered going for overflow-x instead of overflow?

reiv · 2018-03-20T00:52:21Z

@llorca Yes, overflow-x was the first thing I tried and it did not work; clipping still occurred. See also this comment from @giladgray.

llorca · 2018-03-20T04:46:04Z

@reiv going back to the previous 18px line height try, what about:

// no need for single line class
.pt-menu-item {
  line-height: 18px; // math?
  padding: 6px 7px; // with the right variables math
}
.pt-menu-item .pt-icon {
  margin-top: 1px;
}
.pt-menu-item .pt-menu-item-label {
  align-self: flex-start;
}
// maybe some kind of `$vertical-padding-adjustment-size: 1px` variable to do the math above?

this seems to be working solidly on my end, albeit ugly. but if that solves the clipping 🤷‍♂️

reiv · 2018-03-20T04:46:31Z

Set explicit line-height value based on font size

Preview: documentation | landing | table

reiv · 2018-03-20T04:57:47Z

@llorca Ok, cool. 18px might not work for all zoom levels, but I get the idea.

This reverts commit a86df97.

This reverts commit 6779909.

This reverts commit 8fd1085.

reiv · 2018-03-20T06:10:43Z

Add $menu-item-line-height-factor variable

Preview: documentation | landing | table

reiv · 2018-03-20T06:22:11Z

Add comment

Preview: documentation | landing | table

llorca · 2018-03-20T15:34:13Z

that looks like progress! I like the fact that we don't need an extra class

a couple of issues to address:

text is still misaligned in Chrome, because of the uneven line-height (21px). let's get the math to make it even?
it looks like we're having some float padding (4.5px) and float margin (2.5px) which will render blurry on low res screens. let's get the math to round these as well, hopefully it's as easy as tweaking the line height factor variable?

llorca · 2018-03-20T15:35:28Z

packages/core/src/components/menu/_menu.scss

@@ -61,6 +61,7 @@ Styleguide pt-menu
  &::before,
  .pt-icon {
    align-self: flex-start;


that flex-start works for the icon on the left side, but not for the item label on the right. can you also add flex start for .pt-menu-item-label on line 69?

reiv · 2018-03-20T16:00:44Z

Ensure line-height is an even value

Preview: documentation | landing | table

llorca

Ok, I think this is sane. @giladgray for final approval.

Separately, we'll have to fix large menu item style regressions (#2281)

giladgray

this actually looks really good! math is frightening at first, but the change is small and end result is perfect.

one final refactor.

giladgray · 2018-03-20T21:55:50Z

packages/core/src/components/menu/_menu.scss

    color: $pt-icon-color;
  }

  .pt-menu-item-label {
+    align-self: flex-start;


seems we can just change .pt-menu-item to align-items: flex-start now, rather than overriding with align-self here and line 63.

giladgray

actually, i'm going to merge this so I can work on a fix for #2281 and i'll make the align-items change there.

giladgray · 2018-03-20T23:24:27Z

@reiv thanks for your hard work on this! end result is 💯 🔥

reiv added 2 commits March 17, 2018 01:15

Remove line-height from pt-menu-item

d7210dd

Merge branch 'develop' into rv/fix-menuitem-clipping

e60f25d

reiv added 2 commits March 20, 2018 00:01

Revert "Remove line-height from pt-menu-item"

6db0114

This reverts commit d7210dd.

Add CSS class for single-line menu items

8fd1085

Apply line-height directly to menu item label

6779909

llorca reviewed Mar 20, 2018

View reviewed changes

Set explicit line-height value based on font size

a86df97

reiv added 3 commits March 20, 2018 06:01

Revert "Set explicit line-height value based on font size"

5cf1f3f

This reverts commit a86df97.

Revert "Apply line-height directly to menu item label"

85050df

This reverts commit 6779909.

Revert "Add CSS class for single-line menu items"

3e46897

This reverts commit 8fd1085.

Add $menu-item-line-height-factor variable

186b23b

Add comment

be940ed

llorca reviewed Mar 20, 2018

View reviewed changes

reiv added 2 commits March 20, 2018 16:38

Align label with flex-start

e324f3e

Ensure line-height is an even value

325d93b

llorca approved these changes Mar 20, 2018

View reviewed changes

llorca requested a review from giladgray March 20, 2018 21:11

giladgray suggested changes Mar 20, 2018

View reviewed changes

giladgray approved these changes Mar 20, 2018

View reviewed changes

giladgray merged commit 6b107df into palantir:develop Mar 20, 2018

giladgray mentioned this pull request Mar 20, 2018

[menu] fix large menu items #2285

Merged

reiv deleted the rv/fix-menuitem-clipping branch March 21, 2018 01:33

This was referenced Mar 21, 2018

🌟 Prepare Release v2.0.0 🌟 #2290

Closed

Prepare Release v2.0.0-rc.4 #2291

Merged

	// a little bit extra to ensure the height comes out to just over 18px (and browser rounds to 18px)
	$pt-line-height: ($pt-grid-size * 1.8) / $pt-font-size + 0.0001 !default;

[core] Fix MenuItem text clipping #2265

[core] Fix MenuItem text clipping #2265

Conversation

reiv commented Mar 17, 2018 • edited Loading

Fixes #2177

Reviewers should focus on:

Screenshot

reiv commented Mar 19, 2018

Merge branch 'develop' into rv/fix-menuitem-clipping

giladgray commented Mar 19, 2018

llorca commented Mar 19, 2018

reiv commented Mar 19, 2018

llorca commented Mar 19, 2018

llorca commented Mar 19, 2018

reiv commented Mar 19, 2018

llorca commented Mar 19, 2018

reiv commented Mar 19, 2018

Add CSS class for single-line menu items

llorca commented Mar 19, 2018

reiv commented Mar 19, 2018 • edited Loading

llorca commented Mar 19, 2018

reiv commented Mar 19, 2018 • edited Loading

reiv commented Mar 20, 2018

Apply line-height directly to menu item label

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reiv Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reiv Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

llorca commented Mar 20, 2018

reiv commented Mar 20, 2018

llorca commented Mar 20, 2018

reiv commented Mar 20, 2018

Set explicit line-height value based on font size

reiv commented Mar 20, 2018 • edited Loading

reiv commented Mar 20, 2018

Add $menu-item-line-height-factor variable

reiv commented Mar 20, 2018

Add comment

llorca commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reiv commented Mar 20, 2018

Ensure line-height is an even value

llorca left a comment • edited Loading

Choose a reason for hiding this comment

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray left a comment

Choose a reason for hiding this comment

giladgray commented Mar 20, 2018

reiv commented Mar 17, 2018 •

edited

Loading

reiv commented Mar 19, 2018 •

edited

Loading

reiv commented Mar 19, 2018 •

edited

Loading

llorca Mar 20, 2018 •

edited

Loading

reiv Mar 20, 2018 •

edited

Loading

reiv Mar 20, 2018 •

edited

Loading

reiv commented Mar 20, 2018 •

edited

Loading

llorca left a comment •

edited

Loading