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.

Sign up for GitHub

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

[core] Fix MenuItem text clipping #2265

Merged
merged 13 commits into from
Mar 20, 2018
Merged

[core] Fix MenuItem text clipping #2265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d7210dd
Remove line-height from pt-menu-item
reiv Mar 17, 2018
e60f25d
Merge branch 'develop' into rv/fix-menuitem-clipping
reiv Mar 19, 2018
6db0114
Revert "Remove line-height from pt-menu-item"
reiv Mar 19, 2018
8fd1085
Add CSS class for single-line menu items
reiv Mar 19, 2018
6779909
Apply line-height directly to menu item label
reiv Mar 20, 2018
a86df97
Set explicit line-height value based on font size
reiv Mar 20, 2018
5cf1f3f
Revert "Set explicit line-height value based on font size"
reiv Mar 20, 2018
85050df
Revert "Apply line-height directly to menu item label"
reiv Mar 20, 2018
3e46897
Revert "Add CSS class for single-line menu items"
reiv Mar 20, 2018
186b23b
Add $menu-item-line-height-factor variable
reiv Mar 20, 2018
be940ed
Add comment
reiv Mar 20, 2018
e324f3e
Align label with flex-start
reiv Mar 20, 2018
325d93b
Ensure line-height is an even value
reiv Mar 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions packages/core/src/components/menu/_common.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@ $half-grid-size: $pt-grid-size / 2 !default;

$menu-item-border-radius: $pt-border-radius - 1 !default;

// Set line-height of menu items to be a multiple of the font size. This is
// needed because if the line-height does not extend far enough past the font's
// baseline, clipping will occur when the .pt-text-overflow-ellipsis class is
// applied to it (#2177). Also, line-height should be an even value, or content
// will be misaligned by one pixel (Chrome quirk).
$menu-item-line-height-factor: 1.4;
$menu-item-line-height: round($pt-font-size * $menu-item-line-height-factor);
$menu-item-line-height-large: round($pt-font-size-large * $menu-item-line-height-factor);

$menu-min-width: $pt-grid-size * 18 !default;
$menu-item-padding: ($pt-button-height - $pt-icon-size-standard) / 2 !default;
$menu-item-padding-large: ($pt-button-height-large - $pt-icon-size-large) / 2 !default;
$menu-item-padding-vertical: ($pt-button-height - $menu-item-line-height) / 2 !default;
$menu-item-padding-vertical-large:
($pt-button-height-large - $menu-item-line-height-large) / 2 !default;

$menu-background-color: $white !default;
$dark-menu-background-color: $dark-gray4 !default;
Expand All @@ -23,9 +35,9 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa
@include pt-flex-container(row, $menu-item-padding);
align-items: center;
border-radius: $menu-item-border-radius;
padding: $menu-item-padding;
padding: $menu-item-padding-vertical $menu-item-padding;
text-decoration: none;
line-height: $pt-icon-size-standard;
line-height: $menu-item-line-height;
color: inherit;
user-select: none;

Expand Down Expand Up @@ -103,8 +115,8 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa
}

@mixin menu-item-large() {
padding: $menu-item-padding-large $menu-item-padding;
line-height: $pt-icon-size-large;
padding: $menu-item-padding-vertical-large $menu-item-padding;
line-height: $menu-item-line-height-large;
font-size: $pt-font-size-large;
}

Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/components/menu/_menu.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@ Styleguide pt-menu
&::before,
.pt-icon {
align-self: flex-start;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

margin-top: ($menu-item-line-height - $pt-icon-size-standard) / 2;
color: $pt-icon-color;
}

.pt-menu-item-label {
align-self: flex-start;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

color: $pt-text-color-muted;
}

Expand Down Expand Up @@ -99,6 +101,7 @@ Styleguide pt-menu

&::before {
@include pt-icon($pt-icon-size-large);
margin-top: ($menu-item-line-height-large - $pt-icon-size-large) / 2;
margin-right: $menu-item-padding-large;
}
}
Expand Down