Make `MenuItem` responsible for rendering associated submenus

To ensure consistency between different menus that contain items with
submenus, move the responsibility for rendering the submenu and its
container to the `MenuItem` component. The component that renders the
top-level `MenuItem` must supply a `renderSubmenu` prop that renders the
submenu on-demand.

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

@@ -9,7 +9,6 @@ const { withServices } = require('../util/service-context');
 const { copyText } = require('../util/copy-to-clipboard');
 
 const MenuItem = require('./menu-item');
-const Slider = require('./slider');
 
 /**
  * An item in the groups selection menu.
@@ -80,20 +79,18 @@ function GroupListItem({
   const collapseSubmenu = () => onExpand(false);
 
   return (
-    <Fragment>
-      <MenuItem
-        icon={group.logo || 'blank'}
-        iconAlt={orgName(group)}
-        isDisabled={!isSelectable}
-        isExpanded={hasActionMenu ? isExpanded : undefined}
-        isSelected={isSelected}
-        isSubmenuVisible={isExpanded}
-        label={group.name}
-        onClick={isSelectable ? focusGroup : toggleSubmenu}
-        onToggleSubmenu={toggleSubmenu}
-      />
-      <Slider visible={isExpanded}>
-        <div className="group-list-item__submenu">
+    <MenuItem
+      icon={group.logo || 'blank'}
+      iconAlt={orgName(group)}
+      isDisabled={!isSelectable}
+      isExpanded={hasActionMenu ? isExpanded : undefined}
+      isSelected={isSelected}
+      isSubmenuVisible={isExpanded}
+      label={group.name}
+      onClick={isSelectable ? focusGroup : toggleSubmenu}
+      onToggleSubmenu={toggleSubmenu}
+      renderSubmenu={() => (
+        <Fragment>
           <ul onClick={collapseSubmenu}>
             {activityUrl && (
               <li>
@@ -131,9 +128,9 @@ function GroupListItem({
               This group is restricted to specific URLs.
             </p>
           )}
-        </div>
-      </Slider>
-    </Fragment>
+        </Fragment>
+      )}
+    />
   );
 }
 

src/sidebar/components/menu-item.js

 'use strict';
 
 const classnames = require('classnames');
-const { createElement } = require('preact');
+const { Fragment, createElement } = require('preact');
 const propTypes = require('prop-types');
 
 const { onActivate } = require('../util/on-activate');
 
+const Slider = require('./slider');
 const SvgIcon = require('./svg-icon');
 
 /**
@@ -20,9 +21,8 @@ const SvgIcon = require('./svg-icon');
  * The icon can either be an external SVG image, referenced by URL, or a named
  * icon rendered by an `SvgIcon`.
  *
- * For items that have submenus, the `MenuItem` only provides an indicator as
- * to whether the submenu is open. The container is responsible for displaying
- * the submenu items beneath the current item.
+ * For items that have submenus, the `MenuItem` will call the `renderSubmenu`
+ * prop to render the content of the submenu, when the submenu is visible.
  */
 function MenuItem({
   href,
@@ -36,6 +36,7 @@ function MenuItem({
   label,
   onClick,
   onToggleSubmenu,
+  renderSubmenu,
 }) {
   const iconClass = 'menu-item__icon';
   const iconIsUrl = icon && icon.indexOf('/') !== -1;
@@ -58,51 +59,60 @@ function MenuItem({
   const rightIcon = isSubmenuItem ? renderedIcon : null;
 
   return (
-    <div
-      aria-checked={isSelected}
-      className={classnames('menu-item', {
-        'menu-item--submenu': isSubmenuItem,
-        'is-disabled': isDisabled,
-        'is-expanded': isExpanded,
-        'is-selected': isSelected,
-      })}
-      role="menuitem"
-      {...(onClick && onActivate('menuitem', onClick))}
-    >
-      <div className="menu-item__action">
-        {hasLeftIcon && (
-          <div className="menu-item__icon-container">{leftIcon}</div>
-        )}
-        {href && (
-          <a
-            className={labelClass}
-            href={href}
-            target="_blank"
-            rel="noopener noreferrer"
+    // Wrapper element is a `<div>` rather than a `Fragment` to work around
+    // limitations of Enzyme's shallow rendering.
+    <div>
+      <div
+        aria-checked={isSelected}
+        className={classnames('menu-item', {
+          'menu-item--submenu': isSubmenuItem,
+          'is-disabled': isDisabled,
+          'is-expanded': isExpanded,
+          'is-selected': isSelected,
+        })}
+        role="menuitem"
+        {...(onClick && onActivate('menuitem', onClick))}
+      >
+        <div className="menu-item__action">
+          {hasLeftIcon && (
+            <div className="menu-item__icon-container">{leftIcon}</div>
+          )}
+          {href && (
+            <a
+              className={labelClass}
+              href={href}
+              target="_blank"
+              rel="noopener noreferrer"
+            >
+              {label}
+            </a>
+          )}
+          {!href && <span className={labelClass}>{label}</span>}
+          {hasRightIcon && (
+            <div className="menu-item__icon-container">{rightIcon}</div>
+          )}
+        </div>
+        {typeof isSubmenuVisible === 'boolean' && (
+          <div
+            className="menu-item__toggle"
+            // We need to pass strings here rather than just the boolean attribute
+            // because otherwise the attribute will be omitted entirely when
+            // `isSubmenuVisible` is false.
+            aria-expanded={isSubmenuVisible ? 'true' : 'false'}
+            aria-label={`Show actions for ${label}`}
+            {...onActivate('button', onToggleSubmenu)}
           >
-            {label}
-          </a>
-        )}
-        {!href && <span className={labelClass}>{label}</span>}
-        {hasRightIcon && (
-          <div className="menu-item__icon-container">{rightIcon}</div>
+            <SvgIcon
+              name={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
+              className="menu-item__toggle-icon"
+            />
+          </div>
         )}
       </div>
       {typeof isSubmenuVisible === 'boolean' && (
-        <div
-          className="menu-item__toggle"
-          // We need to pass strings here rather than just the boolean attribute
-          // because otherwise the attribute will be omitted entirely when
-          // `isSubmenuVisible` is false.
-          aria-expanded={isSubmenuVisible ? 'true' : 'false'}
-          aria-label={`Show actions for ${label}`}
-          {...onActivate('button', onToggleSubmenu)}
-        >
-          <SvgIcon
-            name={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
-            className="menu-item__toggle-icon"
-          />
-        </div>
+        <Slider visible={isSubmenuVisible}>
+          <div className="menu-item__submenu">{renderSubmenu()}</div>
+        </Slider>
       )}
     </div>
   );
@@ -171,6 +181,12 @@ MenuItem.propTypes = {
    * state of the menu.
    */
   onToggleSubmenu: propTypes.func,
+
+  /**
+   * Function called to render the item's submenu when `isSubmenuVisible`
+   * is `true`.
+   */
+  renderSubmenu: propTypes.func,
 };
 
 module.exports = MenuItem;

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

@@ -69,7 +69,6 @@ describe('GroupListItem', () => {
 
     GroupListItem.$imports.$mock({
       './menu-item': FakeMenuItem,
-      './slider': FakeSlider,
       '../util/copy-to-clipboard': {
         copyText: fakeCopyText,
       },
@@ -236,19 +235,29 @@ describe('GroupListItem', () => {
     assert.isUndefined(wrapper.find('MenuItem').prop('isExpanded'));
   });
 
+  function getSubmenu(wrapper) {
+    const renderSubmenu = wrapper
+      .find('MenuItem')
+      .first()
+      .prop('renderSubmenu');
+    return mount(<div>{renderSubmenu()}</div>);
+  }
+
   it('does not show link to activity page if not available', () => {
     fakeGroup.links.html = null;
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    assert.isFalse(wrapper.exists('MenuItem[label="View group activity"]'));
+    const submenu = getSubmenu(wrapper);
+    assert.isFalse(submenu.exists('MenuItem[label="View group activity"]'));
   });
 
   it('shows link to activity page if available', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    assert.isTrue(wrapper.exists('MenuItem[label="View group activity"]'));
+    const submenu = getSubmenu(wrapper);
+    assert.isTrue(submenu.exists('MenuItem[label="View group activity"]'));
   });
 
   it('does not show "Leave" action if user cannot leave', () => {
@@ -256,7 +265,8 @@ describe('GroupListItem', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    assert.isFalse(wrapper.exists('MenuItem[label="Leave group"]'));
+    const submenu = getSubmenu(wrapper);
+    assert.isFalse(submenu.exists('MenuItem[label="Leave group"]'));
   });
 
   it('shows "Leave" action if user can leave', () => {
@@ -264,14 +274,18 @@ describe('GroupListItem', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    assert.isTrue(wrapper.exists('MenuItem[label="Leave group"]'));
+    const submenu = getSubmenu(wrapper);
+    assert.isTrue(submenu.exists('MenuItem[label="Leave group"]'));
   });
 
   it('prompts to leave group if "Leave" action is clicked', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    clickMenuItem(wrapper, 'Leave group');
+
+    const submenu = getSubmenu(wrapper);
+    clickMenuItem(submenu, 'Leave group');
+
     assert.called(window.confirm);
     assert.notCalled(fakeGroupsService.leave);
   });
@@ -281,7 +295,10 @@ describe('GroupListItem', () => {
       isExpanded: true,
     });
     window.confirm.returns(true);
-    clickMenuItem(wrapper, 'Leave group');
+
+    const submenu = getSubmenu(wrapper);
+    clickMenuItem(submenu, 'Leave group');
+
     assert.called(window.confirm);
     assert.calledWith(fakeGroupsService.leave, fakeGroup.id);
   });
@@ -316,7 +333,9 @@ describe('GroupListItem', () => {
           .prop('isDisabled'),
         expectDisabled
       );
-      assert.equal(wrapper.exists('.group-list-item__footer'), expectDisabled);
+
+      const submenu = getSubmenu(wrapper);
+      assert.equal(submenu.exists('.group-list-item__footer'), expectDisabled);
     });
   });
 
@@ -348,7 +367,8 @@ describe('GroupListItem', () => {
       const wrapper = createGroupListItem(fakeGroup, {
         isExpanded: true,
       });
-      const copyAction = wrapper
+      const submenu = getSubmenu(wrapper);
+      const copyAction = submenu
         .find('MenuItem')
         .filterWhere(n => n.prop('label').startsWith('Copy'));
 
@@ -364,7 +384,7 @@ describe('GroupListItem', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    clickMenuItem(wrapper, 'Copy invite link');
+    clickMenuItem(getSubmenu(wrapper), 'Copy invite link');
     assert.calledWith(fakeCopyText, 'https://annotate.com/groups/groupid');
     assert.calledWith(fakeFlash.info, 'Copied link for "Test"');
   });
@@ -374,7 +394,7 @@ describe('GroupListItem', () => {
     const wrapper = createGroupListItem(fakeGroup, {
       isExpanded: true,
     });
-    clickMenuItem(wrapper, 'Copy invite link');
+    clickMenuItem(getSubmenu(wrapper), 'Copy invite link');
     assert.calledWith(fakeCopyText, 'https://annotate.com/groups/groupid');
     assert.calledWith(fakeFlash.error, 'Unable to copy link');
   });

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

@@ -56,7 +56,11 @@ describe('MenuItem', () => {
   });
 
   it('shows the submenu indicator if `isSubmenuVisible` is a boolean', () => {
-    const wrapper = createMenuItem({ isSubmenuVisible: true });
+    const wrapper = createMenuItem({
+      isSubmenuVisible: true,
+      // eslint-disable-next-line react/display-name
+      renderSubmenu: () => <div>Submenu</div>,
+    });
     assert.isTrue(wrapper.exists('SvgIcon[name="collapse-menu"]'));
 
     wrapper.setProps({ isSubmenuVisible: false });
@@ -70,7 +74,12 @@ describe('MenuItem', () => {
 
   it('calls the `onToggleSubmenu` callback when the submenu toggle is clicked', () => {
     const onToggleSubmenu = sinon.stub();
-    const wrapper = createMenuItem({ isSubmenuVisible: true, onToggleSubmenu });
+    const wrapper = createMenuItem({
+      isSubmenuVisible: true,
+      onToggleSubmenu,
+      // eslint-disable-next-line react/display-name
+      renderSubmenu: () => <div>Submenu</div>,
+    });
     wrapper.find('.menu-item__toggle').simulate('click');
     assert.called(onToggleSubmenu);
   });
@@ -98,4 +107,44 @@ describe('MenuItem', () => {
     // The actual icon for the submenu should be shown on the right.
     assert.equal(iconSpaces.at(1).children().length, 1);
   });
+
+  it('does not render submenu content if `isSubmenuVisible` is undefined', () => {
+    const wrapper = createMenuItem({});
+    assert.isFalse(wrapper.exists('Slider'));
+  });
+
+  it('shows submenu content if `isSubmenuVisible` is true', () => {
+    const wrapper = createMenuItem({
+      isSubmenuVisible: true,
+      // eslint-disable-next-line react/display-name
+      renderSubmenu: () => <div>Submenu content</div>,
+    });
+    assert.equal(wrapper.find('Slider').prop('visible'), true);
+    assert.equal(
+      wrapper
+        .find('Slider')
+        .children()
+        .text(),
+      'Submenu content'
+    );
+  });
+
+  it('hides submenu content if `isSubmenuVisible` is false', () => {
+    const wrapper = createMenuItem({
+      isSubmenuVisible: false,
+      // eslint-disable-next-line react/display-name
+      renderSubmenu: () => <div>Submenu content</div>,
+    });
+    assert.equal(wrapper.find('Slider').prop('visible'), false);
+
+    // The submenu content may still be rendered if the submenu is currently
+    // collapsing.
+    assert.equal(
+      wrapper
+        .find('Slider')
+        .children()
+        .text(),
+      'Submenu content'
+    );
+  });
 });

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;

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

@@ -148,3 +148,8 @@ $menu-item-padding: 10px;
     height: 12px;
   }
 }
+
+// The container for open submenus
+.menu-item__submenu {
+  border-bottom: solid 1px $grey-2;
+}