[core] feat: MenuItem small modifier; extend MenuDivider width (#6269)

Co-authored-by: Charles <charles.perinet@gmail.com>

packages/core/src/common/classes.ts

@@ -200,6 +200,8 @@ export const FORM_GROUP_SUB_LABEL = `${NS}-form-group-sub-label`;
 
 export const MENU = `${NS}-menu`;
 export const MENU_ITEM = `${MENU}-item`;
+export const MENU_ITEM_IS_SELECTABLE = `${MENU_ITEM}-is-selectable`;
+export const MENU_ITEM_SELECTED_ICON = `${MENU_ITEM}-selected-icon`;
 export const MENU_ITEM_ICON = `${MENU_ITEM}-icon`;
 export const MENU_ITEM_LABEL = `${MENU_ITEM}-label`;
 export const MENU_SUBMENU = `${NS}-submenu`;

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

@@ -22,6 +22,11 @@ $menu-item-padding-large: ($pt-button-height-large - $pt-icon-size-large) * 0.5
 $menu-item-padding-vertical: ($pt-button-height - $menu-item-line-height) * 0.5 !default;
 $menu-item-padding-vertical-large:
   ($pt-button-height-large - $menu-item-line-height-large) * 0.5 !default;
+$menu-item-padding-small: ($pt-button-height-small - $pt-icon-size-standard) * 0.5 !default;
+$menu-item-padding-vertical-small:
+  ($pt-button-height-small - $menu-item-line-height) * 0.5 !default;
+$menu-item-selected-icon-spacing: 2px;
+$menu-item-indent: $pt-icon-size-standard + $menu-item-selected-icon-spacing * 2;
 
 $menu-background-color: $white !default;
 $dark-menu-background-color: $dark-gray3 !default;
@@ -70,6 +75,7 @@ $dark-menu-item-intent-colors: (
 
   &::before,
   .#{$ns}-menu-item-icon,
+  .#{$ns}-menu-item-selected-icon,
   .#{$ns}-submenu-icon {
     color: $pt-icon-color;
   }
@@ -83,24 +89,35 @@ $dark-menu-item-intent-colors: (
     @include menu-item-hover();
   }
 
-  &:active,
-  &.#{$ns}-active {
+  // N.B. mouse interaction :active appearance is different from the .#{$ns}-active modifier class.
+  // The latter is often used to indicate keyboard navigation, and it's helpful to distinguish between keyboard
+  // and mouse interactions.
+  &:active {
     background-color: $menu-item-color-active;
 
     .#{$ns}-menu-item-label {
       color: $pt-text-color;
     }
   }
 
-  &.#{$ns}-selected {
-    &,
-    &:hover,
-    &:active {
-      @include menu-item-selected-active(false);
+  &.#{$ns}-active {
+    @include menu-item-active(false);
+  }
+
+  &.#{$ns}-menu-item-is-selectable {
+    padding-left: $menu-item-indent;
+
+    &.#{$ns}-selected {
+      padding-left: 0;
+    }
+
+    .#{$ns}-menu-item-selected-icon {
+      align-self: center;
+      margin: 0 $menu-item-selected-icon-spacing;
     }
   }
 
-  // pt-disable always overrides over styles
+  // disabled class needs to always overrides other styles
   /* stylelint-disable declaration-no-important */
   &.#{$ns}-disabled {
     background-color: inherit !important;
@@ -131,6 +148,7 @@ $dark-menu-item-intent-colors: (
 
   &::before,
   .#{$ns}-menu-item-icon,
+  .#{$ns}-menu-item-selected-icon,
   .#{$ns}-submenu-icon {
     color: $pt-dark-icon-color;
   }
@@ -139,19 +157,18 @@ $dark-menu-item-intent-colors: (
     @include dark-menu-item-hover();
   }
 
-  &:active,
-  &.#{$ns}-active {
+  // N.B. mouse interaction :active appearance is different from the .#{$ns}-active modifier class.
+  &:active {
+    // we can use the same color as light theme due to transparency
+    background-color: $menu-item-color-active;
+
     .#{$ns}-menu-item-label {
       color: $pt-dark-text-color;
     }
   }
 
-  &.#{$ns}-selected {
-    &,
-    &:hover,
-    &:active {
-      @include menu-item-selected-active(true);
-    }
+  &.#{$ns}-active {
+    @include menu-item-active(true);
   }
 
   // pt-disable always overrides over styles
@@ -177,10 +194,6 @@ $dark-menu-item-intent-colors: (
   color: inherit;
   cursor: pointer;
   text-decoration: none;
-
-  &.#{ns}-selected {
-    @include menu-item-selected-active(false);
-  }
 }
 
 @mixin dark-menu-item-hover() {
@@ -191,13 +204,9 @@ $dark-menu-item-intent-colors: (
   .#{$ns}-submenu-icon {
     color: $pt-dark-icon-color;
   }
-
-  &.#{ns}-selected {
-    @include menu-item-selected-active(true);
-  }
 }
 
-@mixin menu-item-selected-active($is-dark) {
+@mixin menu-item-active($is-dark) {
   $intent-colors: $menu-item-intent-colors;
   $background-alpha: 0.1;
 
@@ -215,6 +224,7 @@ $dark-menu-item-intent-colors: (
 
   &::before,
   .#{$ns}-menu-item-icon,
+  .#{$ns}-menu-item-selected-icon,
   .#{$ns}-submenu-icon {
     color: nth(map-get($intent-colors, "primary"), 2);
   }
@@ -244,6 +254,7 @@ $dark-menu-item-intent-colors: (
 
     &::before,
     .#{$ns}-menu-item-icon,
+    .#{$ns}-menu-item-selected-icon,
     .#{$ns}-submenu-icon,
     .#{$ns}-menu-item-label {
       color: inherit;
@@ -272,7 +283,8 @@ $dark-menu-item-intent-colors: (
 @mixin menu-item-large() {
   font-size: $pt-font-size-large;
   line-height: $menu-item-line-height-large;
-  padding: $menu-item-padding-vertical-large $menu-item-padding;
+  padding-bottom: $menu-item-padding-vertical-large;
+  padding-top: $menu-item-padding-vertical-large;
 
   .#{$ns}-menu-item-icon {
     height: $menu-item-line-height-large;
@@ -285,10 +297,16 @@ $dark-menu-item-intent-colors: (
   }
 }
 
+@mixin menu-item-small() {
+  padding-bottom: $menu-item-padding-vertical-small;
+  padding-top: $menu-item-padding-vertical-small;
+}
+
 @mixin menu-divider() {
   border-top: 1px solid $pt-divider-black;
   display: block;
-  margin: $half-grid-size;
+  // use negative margin to make dividers take up full menu width
+  margin: $half-grid-size (-$half-grid-size);
 
   .#{$ns}-dark & {
     border-top-color: $pt-dark-divider-white;
@@ -321,7 +339,7 @@ $dark-menu-item-intent-colors: (
   // a little extra space to avoid clipping descenders (because overflow hidden)
   line-height: $pt-icon-size-standard + 1px;
   margin: 0;
-  padding: $pt-grid-size $menu-item-padding 0 1px;
+  padding: $pt-grid-size $menu-item-padding 0 (1px + $half-grid-size);
 }
 
 @mixin menu-header-large($heading-selector) {

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

@@ -28,7 +28,8 @@ Markup:
   </li>
 </ul>
 
-.#{$ns}-large - Large size (only supported on <code>.#{$ns}-menu</code>)
+.#{$ns}-large - Large size
+.#{$ns}-small - Small size
 
 Styleguide menu
 */
@@ -69,6 +70,10 @@ Styleguide menu
       margin-right: $menu-item-padding-large;
     }
   }
+
+  .#{$ns}-small & {
+    @include menu-item-small();
+  }
 }
 
 button.#{$ns}-menu-item {

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

@@ -27,10 +27,10 @@
         &,
         &:hover,
         &:active {
-          @include menu-item-selected-active(false);
+          @include menu-item-active(false);
 
           .#{$ns}-dark & {
-            @include menu-item-selected-active(true);
+            @include menu-item-active(true);
           }
         }
       }

packages/core/src/components/menu/menu.md

@@ -90,11 +90,21 @@ there is not enough room to the right.
 </Menu>
 ```
 
-@## CSS
+@## CSS API
 
-Menus can be constructed manually using the HTML markup and `@ns-menu-*` classes below. However, you
-should use the menu [React components](#core/components/menu.javscript-api) instead wherever possible,
-as they abstract away the tedious parts of implementing a menu.
+<div class="@ns-callout @ns-intent-warning @ns-icon-warning-sign">
+    <h5 class="@ns-heading">
+
+Deprecated API: use [`<Menu>` and `<MenuItem>`](#core/components/menu)
+
+</h5>
+
+CSS APIs for Blueprint components are considered deprecated, as they are verbose, error-prone, and they
+often fall out of sync as the design system is updated. You should use the React component APIs instead.
+
+</div>
+
+Menus can be constructed manually using the following HTML markup and `@ns-menu-*` classes (available in JS/TS as `Classes.MENU_*`):
 
 * Begin with a `ul.@ns-menu`. Each `li` child denotes a single entry in the menu.
 

packages/core/src/components/menu/menu.tsx

@@ -27,6 +27,9 @@ export interface MenuProps extends Props, React.HTMLAttributes<HTMLUListElement>
     /** Whether the menu items in this menu should use a large appearance. */
     large?: boolean;
 
+    /** Whether the menu items in this menu should use a small appearance. */
+    small?: boolean;
+
     /** Ref handler that receives the HTML `<ul>` element backing this component. */
     ulRef?: React.Ref<HTMLUListElement>;
 }
@@ -40,8 +43,11 @@ export class Menu extends AbstractPureComponent<MenuProps> {
     public static displayName = `${DISPLAYNAME_PREFIX}.Menu`;
 
     public render() {
-        const { className, children, large, ulRef, ...htmlProps } = this.props;
-        const classes = classNames(Classes.MENU, { [Classes.LARGE]: large }, className);
+        const { className, children, large, small, ulRef, ...htmlProps } = this.props;
+        const classes = classNames(className, Classes.MENU, {
+            [Classes.LARGE]: large,
+            [Classes.SMALL]: small,
+        });
         return (
             <ul role="menu" {...htmlProps} className={classes} ref={ulRef}>
                 {children}

packages/core/src/components/menu/menuItem.tsx

@@ -17,7 +17,7 @@
 import classNames from "classnames";
 import * as React from "react";
 
-import { CaretRight } from "@blueprintjs/icons";
+import { CaretRight, SmallTick } from "@blueprintjs/icons";
 
 import { Classes } from "../../common";
 import { ActionProps, DISPLAYNAME_PREFIX, removeNonHTMLProps } from "../../common/props";
@@ -84,29 +84,25 @@ export interface MenuItemProps
      *
      * If `menuitem`, role structure becomes:
      *
-     * `<li role="none"`
-     *     `<a role="menuitem"`
+     * `<li role="none"><a role="menuitem" /></li>`
      *
      * which is proper role structure for a `<ul role="menu"` parent (this is the default `role` of a `Menu`).
      *
      * If `listoption`, role structure becomes:
      *
-     * `<li role="option"`
-     *     `<a role=undefined`
+     * `<li role="option"><a role={undefined} /></li>`
      *
      * which is proper role structure for a `<ul role="listbox"` parent, or a `<select>` parent.
      *
      * If `listitem`, role structure becomes:
      *
-     * `<li role=undefined`
-     *     `<a role=undefined`
+     * `<li role={undefined}><a role={undefined} /></li>`
      *
      * which can be used if this item is within a basic `<ul/>` (or `role="list"`) parent.
      *
      * If `none`, role structure becomes:
      *
-     * `<li role="none"`
-     *     `<a role=undefined`
+     * `<li role="none"><a role={undefined} /></li>`
      *
      * which can be used if wrapping this item in a custom `<li>` parent.
      *
@@ -180,6 +176,7 @@ export const MenuItem: React.FC<MenuItemProps> = React.forwardRef<HTMLLIElement,
         className,
         children,
         disabled,
+        icon,
         intent,
         labelClassName,
         labelElement,
@@ -196,36 +193,34 @@ export const MenuItem: React.FC<MenuItemProps> = React.forwardRef<HTMLLIElement,
         ...htmlProps
     } = props;
 
-    const [liRole, targetRole, icon, ariaSelected] =
+    const [liRole, targetRole, ariaSelected] =
         roleStructure === "listoption" // "listoption": parent has listbox role, or is a <select>
             ? [
                   "option",
                   undefined, // target should have no role
-                  props.icon ?? (selected === undefined ? undefined : selected ? "small-tick" : "blank"),
                   Boolean(selected), // aria-selected prop
               ]
             : roleStructure === "menuitem" // "menuitem": parent has menu role
             ? [
                   "none",
                   "menuitem",
-                  props.icon,
                   undefined, // don't set aria-selected prop
               ]
             : roleStructure === "none" // "none": allows wrapping MenuItem in custom <li>
             ? [
                   "none",
                   undefined, // target should have no role
-                  props.icon,
                   undefined, // don't set aria-selected prop
               ]
             : // roleStructure === "listitem"
               [
                   undefined, // needs no role prop, li is listitem by default
                   undefined,
-                  props.icon,
                   undefined, // don't set aria-selected prop
               ];
 
+    const isSelectable = roleStructure === "listoption";
+    const isSelected = isSelectable && selected;
     const hasIcon = icon != null;
     const hasSubmenu = children != null;
 
@@ -238,7 +233,8 @@ export const MenuItem: React.FC<MenuItemProps> = React.forwardRef<HTMLLIElement,
             [Classes.DISABLED]: disabled,
             // prevent popover from closing when clicking on submenu trigger or disabled item
             [Classes.POPOVER_DISMISS]: shouldDismissPopover && !disabled && !hasSubmenu,
-            [Classes.SELECTED]: active && intentClass === undefined,
+            [Classes.MENU_ITEM_IS_SELECTABLE]: isSelectable,
+            [Classes.SELECTED]: isSelected,
         },
         className,
     );
@@ -263,6 +259,7 @@ export const MenuItem: React.FC<MenuItemProps> = React.forwardRef<HTMLLIElement,
             ...(disabled ? DISABLED_PROPS : {}),
             className: anchorClasses,
         },
+        isSelected ? <SmallTick className={Classes.MENU_ITEM_SELECTED_ICON} /> : undefined,
         hasIcon ? (
             // wrap icon in a <span> in case `icon` is a custom element rather than a built-in icon identifier,
             // so that we always render this class

packages/docs-app/src/examples/core-examples/common/sizeSelect.tsx

@@ -47,3 +47,15 @@ SizeSelect.defaultProps = {
     label: "Size",
     optionLabels: ["Small", "Regular", "Large"],
 };
+
+export function getSizeProp(size: Size) {
+    switch (size) {
+        case "large":
+            return { large: true };
+        case "small":
+            return { small: true };
+        default:
+            // regular is the default
+            return {};
+    }
+}