Update `MenuItem` to take `IconComponent`s, `leftChannelContent`

Previously, `MenuItem`'s `icon` prop accepted a string, which could be:

- the name of a registered icon
- a URL (in which case an `img` with that URL as `src` was rendered)
- the string "blank" (in which case space was added at left)

This became over-complex when migrating to use updated icon components.
Now the `icon` prop expects an `IconComponent`, but `MenuItem` also
takes a `leftChannelContent` prop. This allows consumers to set any
content in the left channel — e.g. an image instead of an icon — and
reduces the complexity of this component.

`GroupListItem` has been updated to set `leftChannelContent` as needed
instead of passing "blank" or a URL to an image as the `icon` prop.

Other components updated to provide `IconComponent`s instead of strings.

src/sidebar/components/Annotation/AnnotationPublishControl.tsx

 import {
   Button,
   CancelIcon,
+  GlobeIcon,
+  GroupsIcon,
+  LockIcon,
   MenuExpandIcon,
 } from '@hypothesis/frontend-shared/lib/next';
 import classnames from 'classnames';
@@ -114,13 +117,13 @@ function AnnotationPublishControl({
             align="left"
           >
             <MenuItem
-              icon={group.type === 'open' ? 'public' : 'groups'}
+              icon={group.type === 'open' ? GlobeIcon : GroupsIcon}
               label={group.name}
               isSelected={!isPrivate}
               onClick={() => onSetPrivate(false)}
             />
             <MenuItem
-              icon="lock"
+              icon={LockIcon}
               label="Only Me"
               isSelected={isPrivate}
               onClick={() => onSetPrivate(true)}

src/sidebar/components/Annotation/test/AnnotationPublishControl-test.js

+import {
+  GlobeIcon,
+  GroupsIcon,
+  LockIcon,
+} from '@hypothesis/frontend-shared/lib/next';
 import { mount } from 'enzyme';
 
 import { checkAccessibility } from '../../../../test-util/accessibility';
@@ -140,7 +145,7 @@ describe('AnnotationPublishControl', () => {
           const wrapper = createAnnotationPublishControl();
           const shareMenuItem = wrapper.find('MenuItem').first();
 
-          assert.equal(shareMenuItem.prop('icon'), 'groups');
+          assert.equal(shareMenuItem.props().icon, GroupsIcon);
         });
       });
 
@@ -153,7 +158,7 @@ describe('AnnotationPublishControl', () => {
           const wrapper = createAnnotationPublishControl();
           const shareMenuItem = wrapper.find('MenuItem').first();
 
-          assert.equal(shareMenuItem.prop('icon'), 'public');
+          assert.equal(shareMenuItem.props().icon, GlobeIcon);
         });
       });
     });
@@ -173,7 +178,7 @@ describe('AnnotationPublishControl', () => {
         const wrapper = createAnnotationPublishControl();
         const privateMenuItem = wrapper.find('MenuItem').at(1);
 
-        assert.equal(privateMenuItem.prop('icon'), 'lock');
+        assert.equal(privateMenuItem.prop('icon'), LockIcon);
       });
 
       it('should have an "Only me" label', () => {

src/sidebar/components/GroupList/GroupList.js

 import classnames from 'classnames';
+import { PlusIcon } from '@hypothesis/frontend-shared/lib/next';
 import { useMemo, useState } from 'preact/hooks';
 
 import { serviceConfig } from '../../config/service-config';
@@ -158,7 +159,11 @@ function GroupList({ settings }) {
       )}
 
       {canCreateNewGroup && (
-        <MenuItem icon="add" href={newGroupLink} label="New private group" />
+        <MenuItem
+          icon={PlusIcon}
+          href={newGroupLink}
+          label="New private group"
+        />
       )}
     </Menu>
   );

src/sidebar/components/GroupList/GroupListItem.js

 import classnames from 'classnames';
+import {
+  CopyIcon,
+  ExternalIcon,
+  LeaveIcon,
+} from '@hypothesis/frontend-shared/lib/next';
 
 import { orgName } from '../../helpers/group-list-item-common';
 import { withServices } from '../../service-context';
@@ -95,15 +100,20 @@ function GroupListItem({
   const copyLinkLabel =
     group.type === 'private' ? 'Copy invite link' : 'Copy activity link';
 
+  const leftChannelContent = group.logo ? (
+    <img className="w-4 h-4" alt={orgName(group)} src={group.logo} />
+  ) : (
+    <span className="sr-only">{orgName(group)}</span>
+  );
+
   return (
     <MenuItem
-      icon={group.logo || 'blank'}
-      iconAlt={orgName(group)}
       isDisabled={!isSelectable}
       isExpanded={hasActionMenu ? isExpanded : false}
       isSelected={isSelected}
       isSubmenuVisible={hasActionMenu ? isExpanded : undefined}
       label={group.name}
+      leftChannelContent={leftChannelContent}
       onClick={isSelectable ? focusGroup : toggleSubmenu}
       onToggleSubmenu={toggleSubmenu}
       submenu={
@@ -113,7 +123,7 @@ function GroupListItem({
               <li>
                 <MenuItem
                   href={activityUrl}
-                  icon="external"
+                  icon={ExternalIcon}
                   isSubmenuItem={true}
                   label="View group activity"
                 />
@@ -123,7 +133,7 @@ function GroupListItem({
               <li>
                 <MenuItem
                   onClick={() => copyLink(activityUrl)}
-                  icon="copy"
+                  icon={CopyIcon}
                   isSubmenuItem={true}
                   label={copyLinkLabel}
                 />
@@ -132,7 +142,7 @@ function GroupListItem({
             {group.canLeave && (
               <li>
                 <MenuItem
-                  icon="leave"
+                  icon={LeaveIcon}
                   isSubmenuItem={true}
                   label="Leave group"
                   onClick={leaveGroup}

src/sidebar/components/GroupList/test/GroupListItem-test.js

@@ -127,9 +127,9 @@ describe('GroupListItem', () => {
       .returns(group.organization.name);
 
     const wrapper = createGroupListItem(group);
-    const altText = wrapper.find('MenuItem').prop('iconAlt');
+    const leftContent = wrapper.find('MenuItem').prop('leftChannelContent');
 
-    assert.equal(altText, group.organization.name);
+    assert.equal(leftContent.props.alt, group.organization.name);
   });
 
   describe('selected state', () => {

src/sidebar/components/MenuItem.tsx

 import classnames from 'classnames';
-import { Icon } from '@hypothesis/frontend-shared';
+import type { IconComponent } from '@hypothesis/frontend-shared/lib/types';
 import {
+  CaretUpIcon,
   MenuExpandIcon,
-  MenuCollapseIcon,
 } from '@hypothesis/frontend-shared/lib/next';
 import type { ComponentChildren, Ref } from 'preact';
 import { useEffect, useRef } from 'preact/hooks';
@@ -21,7 +21,9 @@ function SubmenuToggle({
   isExpanded,
   onToggleSubmenu,
 }: SubmenuToggleProps) {
-  const Icon = isExpanded ? MenuCollapseIcon : MenuExpandIcon;
+  // FIXME: Use `MenuCollapseIcon` instead of `CaretUpIcon` once size
+  // disparities are addressed
+  const Icon = isExpanded ? CaretUpIcon : MenuExpandIcon;
   return (
     <div
       data-testid="submenu-toggle"
@@ -53,10 +55,7 @@ function SubmenuToggle({
       onClick={onToggleSubmenu}
       title={title}
     >
-      <Icon
-        name={isExpanded ? 'collapse-menu' : 'expand-menu'}
-        className="w-3 h-3"
-      />
+      <Icon className="w-3 h-3" />
     </div>
   );
 }
@@ -67,17 +66,13 @@ export type MenuItemProps = {
    * `href` or an `onClick` callback should be supplied.
    */
   href?: string;
-  /** Alt text for icon */
-  iconAlt?: string;
 
   /**
-   * Name or URL of icon to display. If the value is a URL it is displayed using
-   * an `<img>`; if it is a non-URL string it is assumed to be the `name` of a
-   * registered icon. If the property is `"blank"` a blank placeholder is
-   * displayed in place of an icon. The placeholder is useful to keep menu item
-   * labels aligned.
+   * Icon to render for this item. This will show to the left of the item label
+   * unless this is a submenu item, in which case it goes on the right. Ignored
+   * if this is not a submenu item and `leftChannelContent` is also provided.
    */
-  icon?: string;
+  icon?: IconComponent;
 
   /**
    * Dim the label to indicate that this item is not currently available.  The
@@ -108,7 +103,15 @@ export type MenuItemProps = {
    */
   isSubmenuVisible?: boolean;
 
-  label: string;
+  label: ComponentChildren;
+
+  /**
+   * Optional content to render into a left channel. This accommodates small
+   * non-icon images or spacing and will supersede any provided icon if this
+   * is not a submenu item.
+   */
+  leftChannelContent?: ComponentChildren;
+
   onClick?: (e: Event) => void;
   onToggleSubmenu?: (e: Event) => void;
   /**
@@ -138,20 +141,18 @@ export type MenuItemProps = {
  */
 export default function MenuItem({
   href,
-  icon,
-  iconAlt,
+  icon: Icon,
   isDisabled,
   isExpanded,
   isSelected,
   isSubmenuItem,
   isSubmenuVisible,
   label,
+  leftChannelContent,
   onClick,
   onToggleSubmenu,
   submenu,
 }: MenuItemProps) {
-  const iconIsUrl = icon && icon.indexOf('/') !== -1;
-
   const menuItemRef = useRef<HTMLAnchorElement | HTMLDivElement | null>(null);
 
   let focusTimer: number | undefined;
@@ -197,40 +198,27 @@ export default function MenuItem({
     }
   };
 
-  let renderedIcon = null;
-  if (icon && icon !== 'blank') {
-    renderedIcon = iconIsUrl ? (
-      <img className="w-4 h-4" alt={iconAlt} src={icon} />
-    ) : (
-      <Icon name={icon} classes="h-3 w-3" />
-    );
-  }
-  const leftIcon = isSubmenuItem ? null : renderedIcon;
+  const renderedIcon = Icon ? <Icon className="h-3 w-3" /> : null;
+  const leftIcon = !isSubmenuItem ? renderedIcon : null;
   const rightIcon = isSubmenuItem ? renderedIcon : null;
 
-  // MenuItem content layout consists of:
-  // - Sometimes a left item, which may contain an icon or serve as
-  //   an indenting space for label alignment
-  // - Always a label
-  // - Sometimes a right item, which contains an icon (submenu items)
-  // - Sometimes a submenu-toggle control (only if the item has a submenu)
-  const hasLeftItem = leftIcon || isSubmenuItem || icon === 'blank';
-  const hasRightItem = rightIcon && isSubmenuItem;
+  const hasLeftChannel = leftChannelContent || isSubmenuItem || !!leftIcon;
+  const hasRightContent = !!rightIcon;
 
   const menuItemContent = (
     <>
-      {hasLeftItem && (
+      {hasLeftChannel && (
         <div
           className="w-7 flex items-center justify-center"
           data-testid="left-item-container"
         >
-          {leftIcon}
+          {leftChannelContent ?? leftIcon}
         </div>
       )}
       <span className="flex items-center grow whitespace-nowrap px-1">
         {label}
       </span>
-      {hasRightItem && (
+      {hasRightContent && (
         <div
           className="w-8 flex items-center justify-center"
           data-testid="right-item-container"

src/sidebar/components/test/MenuItem-test.js

+import { EditIcon } from '@hypothesis/frontend-shared/lib/next';
 import { mount } from 'enzyme';
 import { act } from 'preact/test-utils';
 
@@ -40,13 +41,6 @@ describe('MenuItem', () => {
       assert.equal(link.prop('rel'), 'noopener noreferrer');
     });
 
-    it('renders an `<img>` icon if an icon URL is provided', () => {
-      const src = 'https://example.com/icon.svg';
-      const wrapper = createMenuItem({ icon: src });
-      const icon = wrapper.find('img');
-      assert.equal(icon.prop('src'), src);
-    });
-
     it('invokes `onClick` callback when pressing `Enter` or space', () => {
       const onClick = sinon.stub();
       const wrapper = createMenuItem({ href: 'https://example.com', onClick });
@@ -93,20 +87,25 @@ describe('MenuItem', () => {
   });
 
   describe('icons for top-level menu items', () => {
-    it('renders an icon if an icon name is provided', () => {
-      const wrapper = createMenuItem({ icon: 'edit' });
-      assert.isTrue(wrapper.exists('Icon[name="edit"]'));
+    it('renders an icon if an icon is provided', () => {
+      const wrapper = createMenuItem({ icon: EditIcon });
+      assert.isTrue(wrapper.exists('EditIcon'));
     });
 
-    it('adds a left container if `icon` is "blank"', () => {
-      const wrapper = createMenuItem({ icon: 'blank' });
-      assert.equal(
-        wrapper.find('[data-testid="left-item-container"]').length,
-        1
+    it('adds a left container if left content is provided', () => {
+      const wrapper = createMenuItem({
+        leftChannelContent: <span>Hi</span>,
+        icon: EditIcon,
+      });
+      const leftChannel = wrapper.find('[data-testid="left-item-container"]');
+      assert.equal(leftChannel.text(), 'Hi');
+      assert.isFalse(
+        wrapper.exists('EditIcon'),
+        'Icon ignored if left channel content provided'
       );
     });
 
-    it('does not add a left container if `icon` is missing', () => {
+    it('does not add a left container if neither icon nor left content provided', () => {
       const wrapper = createMenuItem();
       assert.equal(
         wrapper.find('[data-testid="left-item-container"]').length,
@@ -115,7 +114,7 @@ describe('MenuItem', () => {
     });
 
     it('renders an icon on the left if `icon` provided', () => {
-      const wrapper = createMenuItem({ icon: 'edit' });
+      const wrapper = createMenuItem({ icon: EditIcon });
       const leftItem = wrapper.find('[data-testid="left-item-container"]');
 
       // There should be only one icon space, on the left.
@@ -174,16 +173,12 @@ describe('MenuItem', () => {
 
     it('renders submenu item icons on the right', () => {
       const wrapper = createMenuItem({
-        icon: 'edit',
+        icon: EditIcon,
         isSubmenuItem: true,
         submenu: <div role="menuitem">Submenu content</div>,
       });
       const rightItem = wrapper.find('[data-testid="right-item-container"]');
-
-      assert.equal(rightItem.length, 1);
-
-      // The actual icon for the submenu should be shown on the right.
-      assert.equal(rightItem.at(0).children().length, 1);
+      assert.isTrue(rightItem.find('EditIcon').exists());
     });
 
     it('does not render submenu content if `isSubmenuVisible` is undefined', () => {