Menu item hover / active states (including with pt-intent-*) #565

leebyp · 2017-01-28T01:12:34Z

Fixes #389

Changes proposed in this pull request:

As per the previous discussion thread on #513.

Default hover and active states are grey
.pt-active === :active (background is dependent on .pt-intent-*)
.pt-disabled > .pt-active / pt-intent-*
Update documentation
Update docs site

llorca · 2017-01-28T02:03:22Z

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

  &#{$disabled-selector} {
    color: $pt-dark-text-color-disabled;
  }
 }

+@mixin menu-item-intents($text-color: $pt-intent-text-colors) {


menu-item-intent: https://github.com/palantir/blueprint/blob/master/packages/core/src/components/button/_common.scss#L155

llorca · 2017-01-30T18:28:55Z

Still don't feel great about .pt-selected... We're introducing a new modifier here. I feel like we should make it work with .pt-active.

@pkwi any thoughts on text and background colors?

blueprint-bot · 2017-01-30T19:10:31Z

fix linting

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-30T23:35:51Z

remove pt-selected modifier

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-31T00:28:08Z

use intent-primary active in doc menu items

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-31T00:39:05Z

Update usage docs

Preview: docs | table
Coverage: core | datetime

pkwi · 2017-01-31T15:06:04Z

I think for intents, coloured text with the coloured background on hover makes perfect sense. You want to differentiate these actions with a different colour (for example, red text for "Delete" option). However, you don't want them to be visually dominant, hence the coloured background change happens on only on hover.

leebyp · 2017-02-01T18:59:13Z

@llorca @giladgray @cmslewis we seem pretty solid on this setup, good to review!

dmackerman · 2017-02-02T03:18:57Z

Agreed with @pkwi on the Intents. The docs preview looks great!

llorca · 2017-02-02T03:35:09Z

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

+      &::before,
+      &::after,
+      .pt-menu-item-label {
+        color: $color;


For that loop, can we do it the callout way? https://github.com/palantir/blueprint/blob/master/packages/core/src/components/callout/_callout.scss#L80

The icon should be of the 3rd shade ($pt-intent-colors), while the text is always a shade lighter or darker ($pt-[dark-]intent-text-colors)

Changed the icon colors.

I don't wanna do it exactly the same because the usage requires a delicate mix of .pt-disabled in the _menu.scss file and I want to keep that logic there. The map-get in callout just does the same thing as using $color. We do use both in places - I think you're collating them already in another branch?

llorca · 2017-02-02T03:41:10Z

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

+      }
+
+      &:active,
+      &.pt-active {


.pt-active is very dark... I'd like to propose a small design change where:

.pt-active is nth(map-get($button-intents, $intent), 1)

&:hover:active, .pt-active:active is nth(map-get($button-intents, $intent), 2)

Yeah this is tough...
Having different colors messes with our model of .pt-active === :active elsewhere but for the anticipated usage here, what you propose definitely looks better.

blueprint-bot · 2017-02-02T06:46:04Z

use normal intent colors for icons

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-02T20:14:00Z

update colors for pt-active with pt-intent

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-02T21:52:15Z

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

  color: inherit;

+  &:not(#{$disabled-selector})#{$hover-selector} {


can we plesae avoid this scary specific selector by redefining background in the disabled selector?
happens a few times throughout this change.

giladgray · 2017-02-02T21:52:52Z

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

+      &.pt-active {
+        color: $white;
+
+        &::before,


add &, to this selector list and remove line 87 above.

giladgray · 2017-02-02T21:53:38Z

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

@@ -195,6 +197,9 @@ Markup:
 Styleguide components.menu.css
 */

+$menu-item-color-active: $minimal-button-background-color-active !default;
+$dark-menu-item-color-active: $dark-minimal-button-background-color-active !default;


why are these defined here but $menu-item-color-hover is defined in _common.scss? i think all vars should be in _common.

giladgray · 2017-02-02T21:55:08Z

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

@@ -342,6 +346,15 @@ Styleguide components.menu.css.header
      }
    }

+    &:not(.pt-disabled) {


🔥 look, how hard is it to not have to use :not() here and just override properties when disabled?

this is hard to grok because these are actually the default un-interacted styles (i think?) but the nesting makes me think they're for a modifier, when it's actually the inverse.

fair, we'll need to reset all the icons and text too, but i guess that's only 1 more level down anyway - updated

giladgray · 2017-02-02T21:55:43Z

packages/docs/src/components/navMenu.tsx

-    const itemClasses = classNames(Classes.MENU_ITEM, { [Classes.ACTIVE]: props.isActive });
+    const itemClasses = classNames(Classes.MENU_ITEM, {
+        [Classes.ACTIVE]: props.isActive,
+        [Classes.intentClass(Intent.PRIMARY)]: props.isActive,


[Classes.INTENT_PRIMARY] since you know. same below.

giladgray · 2017-02-02T21:56:30Z

packages/docs/src/styles/_navigator.scss

 }

-.pt-menu-item.pt-active,
-.pt-menu-item:hover {
+.pt-menu-item.pt-active {
  .docs-result-path {


merge this with line above so it's clearly one selector. .pt-menu-item.pt-active .docs-result-path

giladgray · 2017-02-03T01:15:33Z

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

+        }
+
+        .pt-menu-item-label {
+          color: $text-color-disabled;


this actually seems like an appropriate use for !important: disabled beats everything else, so it can force its styles to override more specific selectors. should allow you to revert most of this 👍

giladgray · 2017-02-03T01:16:58Z

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

  }

  &.pt-disabled {
    // override global a:focus styles
    outline: none;
+    background-color: inherit;
    cursor: not-allowed;
    color: $pt-text-color-disabled;


!importants here, with a nice comment explaining how important it is

leebyp · 2017-02-03T20:06:16Z

chatted with @giladgray and trying with !important to overrride. noting that if you use .pt-disabled that's what you're buying into.

blueprint-bot · 2017-02-03T20:09:24Z

use important instead of convoluted css selectors

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-03T21:03:28Z

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

    color: $pt-dark-text-color-disabled;
  }
 }

+@mixin menu-item-intent($text-color: $pt-intent-text-colors) {


this should be text-colors (plural)

blueprint-bot · 2017-02-03T21:21:37Z

cleanup

Preview: docs | landing | table
Coverage: core | datetime

leebyp added 3 commits January 27, 2017 15:29

remove specific docs menu-item stylings

7801df8

light colors changes

386eb66

dark color changes

c469553

leebyp added the Status: in discussion label Jan 28, 2017

llorca reviewed Jan 28, 2017

View reviewed changes

fix linting

143505a

remove pt-selected modifier

91f6ca9

leebyp removed the Status: in discussion label Jan 30, 2017

use intent-primary active in doc menu items

c37f98a

Update usage docs

c329a66

leebyp changed the title (WIP) Menu item pt-selected and hover / active states for pt-intent-* (WIP) Menu item hover / active states (including with pt-intent-*) Jan 31, 2017

leebyp changed the title ~~(WIP) Menu item hover / active states (including with pt-intent-*)~~ Menu item hover / active states (including with pt-intent-*) Jan 31, 2017

llorca reviewed Feb 2, 2017

View reviewed changes

use normal intent colors for icons

0895d96

update colors for pt-active with pt-intent

a4b68e3

giladgray suggested changes Feb 2, 2017

View reviewed changes

leebyp added 2 commits February 2, 2017 16:21

cleanup

b6df4c9

remove not(disabled) from css

9668285

giladgray suggested changes Feb 3, 2017

View reviewed changes

dark disabled without intent disabled background

8ced7ef

giladgray added the P0 label Feb 3, 2017

giladgray mentioned this pull request Feb 3, 2017

Fix z-index stack for .pt-button-group and .pt-control-group elements #592

Merged

2 tasks

use important instead of convoluted css selectors

2d34b9c

giladgray approved these changes Feb 3, 2017

View reviewed changes

cleanup

0bc08a5

leebyp merged commit 6050dde into master Feb 3, 2017

leebyp deleted the bl/menu-item-pt-selected branch February 3, 2017 21:45

giladgray mentioned this pull request Feb 3, 2017

Prepare Release v1.8.0 #616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu item hover / active states (including with pt-intent-*) #565

Menu item hover / active states (including with pt-intent-*) #565

leebyp commented Jan 28, 2017 •

edited

llorca Jan 28, 2017

llorca commented Jan 30, 2017

blueprint-bot commented Jan 30, 2017

blueprint-bot commented Jan 30, 2017

blueprint-bot commented Jan 31, 2017

blueprint-bot commented Jan 31, 2017

pkwi commented Jan 31, 2017

leebyp commented Feb 1, 2017

dmackerman commented Feb 2, 2017

llorca Feb 2, 2017

leebyp Feb 2, 2017

llorca Feb 2, 2017

leebyp Feb 2, 2017

blueprint-bot commented Feb 2, 2017

blueprint-bot commented Feb 2, 2017

giladgray Feb 2, 2017

giladgray Feb 2, 2017

giladgray Feb 2, 2017

giladgray Feb 2, 2017

leebyp Feb 3, 2017

giladgray Feb 2, 2017

giladgray Feb 2, 2017

giladgray Feb 3, 2017

giladgray Feb 3, 2017

leebyp commented Feb 3, 2017

blueprint-bot commented Feb 3, 2017

giladgray Feb 3, 2017

blueprint-bot commented Feb 3, 2017

		color: inherit;

		&:not(#{$disabled-selector})#{$hover-selector} {

Menu item hover / active states (including with pt-intent-*) #565

Menu item hover / active states (including with pt-intent-*) #565

Conversation

leebyp commented Jan 28, 2017 • edited

Fixes #389

Changes proposed in this pull request:

Choose a reason for hiding this comment

llorca commented Jan 30, 2017

blueprint-bot commented Jan 30, 2017

fix linting

blueprint-bot commented Jan 30, 2017

remove pt-selected modifier

blueprint-bot commented Jan 31, 2017

use intent-primary active in doc menu items

blueprint-bot commented Jan 31, 2017

Update usage docs

pkwi commented Jan 31, 2017

leebyp commented Feb 1, 2017

dmackerman commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Feb 2, 2017

use normal intent colors for icons

blueprint-bot commented Feb 2, 2017

update colors for pt-active with pt-intent

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyp commented Feb 3, 2017

blueprint-bot commented Feb 3, 2017

use important instead of convoluted css selectors

Choose a reason for hiding this comment

blueprint-bot commented Feb 3, 2017

cleanup

leebyp commented Jan 28, 2017 •

edited