Merge pull request #1179 from hypothesis/group-list-updates

Update group menu styles

src/images/icons/external.svg

+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" aria-hidden="true" focusable="false" class="Icon Icon--external"><g fill-rule="evenodd"><path fill="none" fill-rule="evenodd" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M7 3h6v6m-1-5l-9 9 9-9z"></path><rect fill="none" stroke="none" x="0" y="0" width="16" height="16"></rect></g></svg>
\ No newline at end of file

src/sidebar/components/group-list-item.js

@@ -70,7 +70,7 @@ function GroupListItem({
   return (
     <Fragment>
       <MenuItem
-        icon={group.logo || null}
+        icon={group.logo || 'blank'}
         iconAlt={orgName(group)}
         isDisabled={!isSelectable}
         isExpanded={isExpanded}
@@ -81,13 +81,13 @@ function GroupListItem({
         onToggleSubmenu={toggleSubmenu}
       />
       {isExpanded && (
-        <Fragment>
+        <div className="group-list-item__submenu">
           <ul onClick={collapseSubmenu}>
             {activityUrl && (
               <li>
                 <MenuItem
                   href={activityUrl}
-                  icon="share"
+                  icon="external"
                   isSubmenuItem={true}
                   label="View group activity"
                 />
@@ -109,7 +109,7 @@ function GroupListItem({
               This group is restricted to specific URLs.
             </p>
           )}
-        </Fragment>
+        </div>
       )}
     </Fragment>
   );

src/sidebar/components/menu-item.js

@@ -43,6 +43,20 @@ function MenuItem({
     'menu-item__label--submenu': isSubmenuItem,
   });
 
+  const hasLeftIcon = icon || isSubmenuItem;
+  const hasRightIcon = icon && isSubmenuItem;
+
+  let renderedIcon = null;
+  if (icon !== 'blank') {
+    renderedIcon = iconIsUrl ? (
+      <img className={iconClass} alt={iconAlt} src={icon} />
+    ) : (
+      <SvgIcon name={icon} className="menu-item__icon" />
+    );
+  }
+  const leftIcon = isSubmenuItem ? null : renderedIcon;
+  const rightIcon = isSubmenuItem ? renderedIcon : null;
+
   return (
     <div
       aria-checked={isSelected}
@@ -55,15 +69,8 @@ function MenuItem({
       role="menuitem"
       {...(onClick && onActivate('menuitem', onClick))}
     >
-      {icon !== undefined && (
-        <div className="menu-item__icon-container">
-          {icon &&
-            (iconIsUrl ? (
-              <img className={iconClass} alt={iconAlt} src={icon} />
-            ) : (
-              <SvgIcon name={icon} className="menu-item__icon" />
-            ))}
-        </div>
+      {hasLeftIcon && (
+        <div className="menu-item__icon-container">{leftIcon}</div>
       )}
       {href && (
         <a
@@ -76,6 +83,9 @@ function MenuItem({
         </a>
       )}
       {!href && <span className={labelClass}>{label}</span>}
+      {hasRightIcon && (
+        <div className="menu-item__icon-container">{rightIcon}</div>
+      )}
       {typeof isSubmenuVisible === 'boolean' && (
         <div
           className="menu-item__toggle"
@@ -110,8 +120,8 @@ MenuItem.propTypes = {
    * Name or URL of icon to display. If the value is a URL it is displayed
    * using an `<img>`, if it is a name it is displayed using `SvgIcon`.
    *
-   * If the property is `null` a blank placeholder is displayed in place of an
-   * icon. If the property is omitted, no placeholder is displayed.
+   * If the property is `"blank"` a blank placeholder is displayed in place of an
+   * icon. If the property is falsey, no placeholder is displayed.
    * The placeholder is useful to keep menu item labels aligned in a list if
    * some items have icons and others do not.
    */

src/sidebar/components/svg-icon.js

@@ -11,6 +11,7 @@ const icons = {
   'expand-menu': require('../../images/icons/expand-menu.svg'),
   copy: require('../../images/icons/copy.svg'),
   cursor: require('../../images/icons/cursor.svg'),
+  external: require('../../images/icons/external.svg'),
   help: require('../../images/icons/help.svg'),
   leave: require('../../images/icons/leave.svg'),
   refresh: require('../../images/icons/refresh.svg'),

src/sidebar/components/test/menu-item-test.js

@@ -42,6 +42,19 @@ describe('MenuItem', () => {
     assert.isTrue(wrapper.exists('SvgIcon[name="an-svg-icon"]'));
   });
 
+  it('renders a blank space for an icon if `icon` is "blank"', () => {
+    const wrapper = createMenuItem({ icon: 'blank' });
+    const iconSpace = wrapper.find('.menu-item__icon-container');
+    assert.equal(iconSpace.length, 1);
+    assert.equal(iconSpace.children().length, 0);
+  });
+
+  it('does not render a space for an icon if `icon` is missing', () => {
+    const wrapper = createMenuItem();
+    const iconSpace = wrapper.find('.menu-item__icon-container');
+    assert.equal(iconSpace.length, 0);
+  });
+
   it('shows the submenu indicator if `isSubmenuVisible` is a boolean', () => {
     const wrapper = createMenuItem({ isSubmenuVisible: true });
     assert.isTrue(wrapper.exists('SvgIcon[name="collapse-menu"]'));
@@ -61,4 +74,28 @@ describe('MenuItem', () => {
     wrapper.find('.menu-item__toggle').simulate('click');
     assert.called(onToggleSubmenu);
   });
+
+  it('renders top-level menu item icons on the left', () => {
+    const wrapper = createMenuItem({ icon: 'an-svg-icon' });
+    const iconSpaces = wrapper.find('.menu-item__icon-container');
+
+    // There should be only one icon space, on the left.
+    assert.equal(iconSpaces.length, 1);
+    assert.equal(iconSpaces.at(0).children().length, 1);
+  });
+
+  it('renders submenu item icons on the right', () => {
+    const wrapper = createMenuItem({
+      icon: 'an-svg-icon',
+      isSubmenuItem: true,
+    });
+    const iconSpaces = wrapper.find('.menu-item__icon-container');
+    assert.equal(iconSpaces.length, 2);
+
+    // There should be a space for the parent item's icon.
+    assert.equal(iconSpaces.at(0).children().length, 0);
+
+    // The actual icon for the submenu should be shown on the right.
+    assert.equal(iconSpaces.at(1).children().length, 1);
+  });
 });

src/styles/sidebar/components/group-list-item.scss

+.group-list-item__submenu {
+  border-bottom: solid 1px $grey-2;
+}
+
 // Footer to display at the bottom of a menu item.
 .group-list-item__footer {
   background-color: $grey-1;
@@ -8,4 +12,8 @@
   // Align the left edge of the footer text with menu item labels above.
   padding-left: 35px;
   white-space: normal;
+
+  // Override default hyphenation which is applied to all `p` elements in the
+  // app.
+  hyphens: none;
 }

src/styles/sidebar/components/menu-item.scss

@@ -4,9 +4,11 @@ $menu-item-padding: 10px;
   $item-padding: $menu-item-padding;
   $item-height: 40px;
 
+  color: $grey-6;
   display: flex;
   flex-direction: row;
   flex-grow: 1;
+  font-weight: 500;
   align-items: center;
   min-width: 150px;
   min-height: $item-height;
@@ -18,19 +20,24 @@ $menu-item-padding: 10px;
   // Prevent menu item text from being selected when user toggles menu.
   user-select: none;
 
+  &:hover {
+    background: $grey-1;
+  }
+
   // An item in a submenu associated with a top-level item.
   &--submenu {
     min-height: $item-height - 10px;
     background: $grey-1;
+    color: mix($grey-5, $grey-6, $weight: 50%);
+    font-weight: normal;
     &:hover {
-      background: $grey-1;
+      // Since submenu items already have a light grey background, we need to
+      // use a slightly darker grey as the hover state.
+      background: $grey-3;
+      color: $grey-6;
     }
   }
 
-  &:hover {
-    background: $grey-1;
-  }
-
   &.is-disabled {
     .menu-item__label {
       color: $grey-4;
@@ -39,6 +46,13 @@ $menu-item-padding: 10px;
 
   &.is-expanded {
     background: $grey-1;
+
+    // Since expanded items already have a light grey background, we need to
+    // make the hover state a little darker. This should match submenu items'
+    // hover state.
+    &:hover {
+      background: $grey-3;
+    }
   }
 
   &.is-selected {
@@ -55,7 +69,7 @@ $menu-item-padding: 10px;
 }
 
 .menu-item__icon {
-  color: $color-gray;
+  color: inherit;
   display: inline-block;
   margin-right: 4px;
   position: relative;
@@ -75,16 +89,12 @@ $menu-item-padding: 10px;
   white-space: nowrap;
   flex-grow: 1;
   flex-shrink: 1;
-  font-weight: 500;
+  font-weight: inherit;
 
   padding-right: $menu-item-padding;
 
   &--submenu {
-    font-weight: 300;
-  }
-
-  &:hover {
-    color: $brand;
+    font-weight: normal;
   }
 }
 
@@ -93,6 +103,7 @@ $menu-item-padding: 10px;
 .menu-item__toggle {
   @include outline-on-keyboard-focus;
 
+  color: $grey-4;
   display: flex;
   flex-direction: column;
   justify-content: center;
@@ -112,10 +123,11 @@ $menu-item-padding: 10px;
 
   &:hover {
     background-color: $grey-3;
+    color: $grey-5;
   }
 
   &-icon {
-    width: 10px;
-    height: 10px;
+    width: 12px;
+    height: 12px;
   }
 }

src/styles/sidebar/components/menu-section.scss

 .menu-section__content {
-  border-bottom: solid 1px rgba(0, 0, 0, 0.15);
+  border-bottom: solid 1px $grey-3;
 }
 
 .menu-section__heading {
@@ -7,6 +7,7 @@
   font-size: $body1-font-size;
   line-height: 1;
   margin: 1px 1px 0;
+  margin-bottom: 10px;
   padding: 12px 10px 0;
   text-transform: uppercase;
 }

src/styles/sidebar/components/menu.scss

@@ -54,6 +54,7 @@
   background-color: white;
   border: 1px solid $grey-3;
   box-shadow: 0 1px 1px rgba(0, 0, 0, 0.1);
+  font-size: $body2-font-size;
   position: absolute;
   top: calc(100% + 5px);