From d7210ddd6e960777370aabc2f19f5e0597519bad Mon Sep 17 00:00:00 2001 From: reiv Date: Sat, 17 Mar 2018 01:15:00 +0100 Subject: [PATCH 01/12] Remove line-height from pt-menu-item --- packages/core/src/components/menu/_common.scss | 1 - packages/core/src/components/menu/_menu.scss | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 1c4fcc3aaa..47b3a3a90a 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -25,7 +25,6 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa border-radius: $menu-item-border-radius; padding: $menu-item-padding; text-decoration: none; - line-height: $pt-icon-size-standard; color: inherit; user-select: none; diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 9e61b6a2c8..1355582675 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -60,7 +60,7 @@ Styleguide pt-menu &::before, .pt-icon { - align-self: flex-start; + align-self: center; color: $pt-icon-color; } From 6db0114a1e1eb7b1fa87ee7ab56c2cd259b2d2ec Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 00:01:45 +0100 Subject: [PATCH 02/12] Revert "Remove line-height from pt-menu-item" This reverts commit d7210ddd6e960777370aabc2f19f5e0597519bad. --- packages/core/src/components/menu/_common.scss | 1 + packages/core/src/components/menu/_menu.scss | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 47b3a3a90a..1c4fcc3aaa 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -25,6 +25,7 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa border-radius: $menu-item-border-radius; padding: $menu-item-padding; text-decoration: none; + line-height: $pt-icon-size-standard; color: inherit; user-select: none; diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 1355582675..9e61b6a2c8 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -60,7 +60,7 @@ Styleguide pt-menu &::before, .pt-icon { - align-self: center; + align-self: flex-start; color: $pt-icon-color; } From 8fd108582f76eb96b8479f6d837bda0ab7624452 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 00:27:32 +0100 Subject: [PATCH 03/12] Add CSS class for single-line menu items --- packages/core/src/common/classes.ts | 1 + packages/core/src/components/menu/_common.scss | 9 +++++++++ packages/core/src/components/menu/menuItem.tsx | 2 ++ 3 files changed, 12 insertions(+) diff --git a/packages/core/src/common/classes.ts b/packages/core/src/common/classes.ts index 9a15526870..a7463f3876 100644 --- a/packages/core/src/common/classes.ts +++ b/packages/core/src/common/classes.ts @@ -116,6 +116,7 @@ export const FORM_HELPER_TEXT = "pt-form-helper-text"; export const MENU = "pt-menu"; export const MENU_ITEM = "pt-menu-item"; export const MENU_ITEM_LABEL = "pt-menu-item-label"; +export const MENU_ITEM_SINGLE_LINE = "pt-menu-item-single-line"; export const MENU_SUBMENU = "pt-submenu"; export const MENU_DIVIDER = "pt-menu-divider"; export const MENU_HEADER = "pt-menu-header"; diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 1c4fcc3aaa..62ef886ebe 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/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 { + // Special case where text is wrapped in pt-text-overflow-ellipsis, which + // sets overflow: hidden. With a non-default line-height, this causes text + // to be clipped. Instead, we set the height directly to adhere to the + // grid. + height: $pt-grid-size * 3; + line-height: normal; + } + .pt-dark & { @include dark-menu-item($disabled-selector, $hover-selector); } diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 7d309db77e..3db91099ce 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -96,6 +96,8 @@ export class MenuItem extends React.PureComponent Date: Tue, 20 Mar 2018 01:12:41 +0100 Subject: [PATCH 04/12] Apply line-height directly to menu item label --- packages/core/src/components/menu/_menu.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 9e61b6a2c8..cb317d6275 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -65,6 +65,7 @@ Styleguide pt-menu } .pt-menu-item-label { + line-height: $pt-icon-size-standard; color: $pt-text-color-muted; } From a86df979d859c9368ee2fb1ed920c0c8d04b3934 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 05:34:02 +0100 Subject: [PATCH 05/12] Set explicit line-height value based on font size --- packages/core/src/components/menu/_common.scss | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 62ef886ebe..1d3f98df89 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -47,11 +47,12 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa &.pt-menu-item-single-line { // Special case where text is wrapped in pt-text-overflow-ellipsis, which - // sets overflow: hidden. With a non-default line-height, this causes text - // to be clipped. Instead, we set the height directly to adhere to the - // grid. + // sets overflow: hidden. If line-height does not extend far enough past the + // font's baseline, this causes text to be clipped, so we bump it up here. + // Then, rather than relying on line-height, we set the element's height + // directly to adhere to the grid. height: $pt-grid-size * 3; - line-height: normal; + line-height: $pt-font-size * 2; } .pt-dark & { From 5cf1f3f61d02dbe0dbe7839316cba52c540f7b35 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 06:01:08 +0100 Subject: [PATCH 06/12] Revert "Set explicit line-height value based on font size" This reverts commit a86df979d859c9368ee2fb1ed920c0c8d04b3934. --- packages/core/src/components/menu/_common.scss | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 1d3f98df89..62ef886ebe 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -47,12 +47,11 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa &.pt-menu-item-single-line { // Special case where text is wrapped in pt-text-overflow-ellipsis, which - // sets overflow: hidden. If line-height does not extend far enough past the - // font's baseline, this causes text to be clipped, so we bump it up here. - // Then, rather than relying on line-height, we set the element's height - // directly to adhere to the grid. + // sets overflow: hidden. With a non-default line-height, this causes text + // to be clipped. Instead, we set the height directly to adhere to the + // grid. height: $pt-grid-size * 3; - line-height: $pt-font-size * 2; + line-height: normal; } .pt-dark & { From 85050df71d71f6ed5f83141f5638ac56a730364e Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 06:01:22 +0100 Subject: [PATCH 07/12] Revert "Apply line-height directly to menu item label" This reverts commit 6779909c9fcc112ababf2338c230c8ed64e9c8d8. --- packages/core/src/components/menu/_menu.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index cb317d6275..9e61b6a2c8 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -65,7 +65,6 @@ Styleguide pt-menu } .pt-menu-item-label { - line-height: $pt-icon-size-standard; color: $pt-text-color-muted; } From 3e46897a205cba40b88b035e41feebf84cf9b7a0 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 06:01:58 +0100 Subject: [PATCH 08/12] Revert "Add CSS class for single-line menu items" This reverts commit 8fd108582f76eb96b8479f6d837bda0ab7624452. --- packages/core/src/common/classes.ts | 1 - packages/core/src/components/menu/_common.scss | 9 --------- packages/core/src/components/menu/menuItem.tsx | 2 -- 3 files changed, 12 deletions(-) diff --git a/packages/core/src/common/classes.ts b/packages/core/src/common/classes.ts index a7463f3876..9a15526870 100644 --- a/packages/core/src/common/classes.ts +++ b/packages/core/src/common/classes.ts @@ -116,7 +116,6 @@ export const FORM_HELPER_TEXT = "pt-form-helper-text"; export const MENU = "pt-menu"; export const MENU_ITEM = "pt-menu-item"; export const MENU_ITEM_LABEL = "pt-menu-item-label"; -export const MENU_ITEM_SINGLE_LINE = "pt-menu-item-single-line"; export const MENU_SUBMENU = "pt-submenu"; export const MENU_DIVIDER = "pt-menu-divider"; export const MENU_HEADER = "pt-menu-header"; diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 62ef886ebe..1c4fcc3aaa 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -45,15 +45,6 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa color: $pt-text-color-disabled; } - &.pt-menu-item-single-line { - // Special case where text is wrapped in pt-text-overflow-ellipsis, which - // sets overflow: hidden. With a non-default line-height, this causes text - // to be clipped. Instead, we set the height directly to adhere to the - // grid. - height: $pt-grid-size * 3; - line-height: normal; - } - .pt-dark & { @include dark-menu-item($disabled-selector, $hover-selector); } diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 3db91099ce..7d309db77e 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -96,8 +96,6 @@ export class MenuItem extends React.PureComponent Date: Tue, 20 Mar 2018 07:04:28 +0100 Subject: [PATCH 09/12] Add $menu-item-line-height-factor variable --- packages/core/src/components/menu/_common.scss | 15 +++++++++++---- packages/core/src/components/menu/_menu.scss | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index 1c4fcc3aaa..e1220a3134 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -5,9 +5,16 @@ $half-grid-size: $pt-grid-size / 2 !default; $menu-item-border-radius: $pt-border-radius - 1 !default; +$menu-item-line-height-factor: 1.5; +$menu-item-line-height: $pt-font-size * $menu-item-line-height-factor; +$menu-item-line-height-large: $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; @@ -23,9 +30,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; @@ -103,8 +110,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; } diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 9e61b6a2c8..3edc79f454 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -61,6 +61,7 @@ Styleguide pt-menu &::before, .pt-icon { align-self: flex-start; + margin-top: ($menu-item-line-height - $pt-icon-size-standard) / 2; color: $pt-icon-color; } @@ -99,6 +100,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; } } From be940ed004de2153293c52f73835179091758cfd Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 07:16:57 +0100 Subject: [PATCH 10/12] Add comment --- packages/core/src/components/menu/_common.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index e1220a3134..de04073c78 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -5,6 +5,10 @@ $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). $menu-item-line-height-factor: 1.5; $menu-item-line-height: $pt-font-size * $menu-item-line-height-factor; $menu-item-line-height-large: $pt-font-size-large * $menu-item-line-height-factor; From e324f3e1969de538df1833d9abd63d4c7d99d4e0 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 16:38:23 +0100 Subject: [PATCH 11/12] Align label with flex-start --- packages/core/src/components/menu/_menu.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 3edc79f454..fa630b959d 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -66,6 +66,7 @@ Styleguide pt-menu } .pt-menu-item-label { + align-self: flex-start; color: $pt-text-color-muted; } From 325d93bc6be9de13f5ee6598067e3629f04841a2 Mon Sep 17 00:00:00 2001 From: reiv Date: Tue, 20 Mar 2018 16:55:12 +0100 Subject: [PATCH 12/12] Ensure line-height is an even value --- packages/core/src/components/menu/_common.scss | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/core/src/components/menu/_common.scss b/packages/core/src/components/menu/_common.scss index de04073c78..92b1a6d8bf 100644 --- a/packages/core/src/components/menu/_common.scss +++ b/packages/core/src/components/menu/_common.scss @@ -8,10 +8,11 @@ $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). -$menu-item-line-height-factor: 1.5; -$menu-item-line-height: $pt-font-size * $menu-item-line-height-factor; -$menu-item-line-height-large: $pt-font-size-large * $menu-item-line-height-factor; +// 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;