Refactor and improve design of tag lists and related components

src/sidebar/components/Annotation/AnnotationBody.js

-import { LabeledButton } from '@hypothesis/frontend-shared';
-import { useState } from 'preact/hooks';
+import { Icon, LabeledButton } from '@hypothesis/frontend-shared';
+import classnames from 'classnames';
+import { useMemo, useState } from 'preact/hooks';
 
 import { useStoreProxy } from '../../store/use-store';
+import { isThirdPartyUser } from '../../helpers/account-id';
 import { isHidden } from '../../helpers/annotation-metadata';
 import { withServices } from '../../service-context';
 import { applyTheme } from '../../helpers/theme';
@@ -9,6 +11,7 @@ import { applyTheme } from '../../helpers/theme';
 import Excerpt from '../Excerpt';
 import MarkdownView from '../MarkdownView';
 import TagList from '../TagList';
+import TagListItem from '../TagListItem';
 
 /**
  * @typedef {import("../../../types/api").Annotation} Annotation
@@ -36,6 +39,7 @@ function AnnotationBody({ annotation, settings }) {
   const [isCollapsible, setIsCollapsible] = useState(false);
 
   const store = useStoreProxy();
+  const defaultAuthority = store.defaultAuthority();
   const draft = store.getDraft(annotation);
 
   const toggleText = isCollapsed ? 'More' : 'Less';
@@ -48,8 +52,20 @@ function AnnotationBody({ annotation, settings }) {
 
   const textStyle = applyTheme(['annotationFontFamily'], settings);
 
+  const shouldLinkTags = useMemo(
+    () => annotation && !isThirdPartyUser(annotation?.user, defaultAuthority),
+    [annotation, defaultAuthority]
+  );
+
+  /**
+   * @param {string} tag
+   */
+  const createTagSearchURL = tag => {
+    return store.getLink('search.tag', { tag });
+  };
+
   return (
-    <div className="hyp-u-vertical-spacing--2">
+    <div className="space-y-4">
       {showExcerpt && (
         <Excerpt
           collapse={isCollapsed}
@@ -63,24 +79,55 @@ function AnnotationBody({ annotation, settings }) {
             textStyle={textStyle}
             markdown={text}
             textClass={{
-              AnnotationBody__text: true,
               'p-redacted-content': isHidden(annotation),
             }}
           />
         </Excerpt>
       )}
+      {(isCollapsible || showTagList) && (
+        <div className="flex flex-row gap-x-2">
+          <div className="grow">
+            {showTagList && (
+              <TagList>
+                {tags.map(tag => {
+                  return (
+                    <TagListItem
+                      key={tag}
+                      tag={tag}
+                      href={
+                        shouldLinkTags ? createTagSearchURL(tag) : undefined
+                      }
+                    />
+                  );
+                })}
+              </TagList>
+            )}
+          </div>
           {isCollapsible && (
-        <div className="hyp-u-layout-row--justify-right">
+            <div>
               <LabeledButton
+                classes={classnames(
+                  // Pull this button up toward the bottom of the excerpt content
+                  '-mt-3',
+                  'text-grey-7 font-normal'
+                )}
                 expanded={!isCollapsed}
                 onClick={() => setIsCollapsed(!isCollapsed)}
                 title={`Toggle visibility of full annotation text: Show ${toggleText}`}
               >
-            {toggleText}
+                <div className="flex items-center gap-x-2">
+                  <Icon
+                    classes="!text-tiny"
+                    name={isCollapsed ? 'expand' : 'collapse'}
+                    title={isCollapsed ? 'expand' : 'collapse'}
+                  />
+                  <div>{toggleText}</div>
+                </div>
               </LabeledButton>
             </div>
           )}
-      {showTagList && <TagList annotation={annotation} tags={tags} />}
+        </div>
+      )}
     </div>
   );
 }

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

@@ -11,6 +11,7 @@ import AnnotationBody, { $imports } from '../AnnotationBody';
 describe('AnnotationBody', () => {
   let fakeAnnotation;
   let fakeApplyTheme;
+  let fakeIsThirdPartyUser;
   let fakeSettings;
 
   // Inject dependency mocks
@@ -44,14 +45,20 @@ describe('AnnotationBody', () => {
     fakeAnnotation.text = 'some text here';
     fakeAnnotation.tags = ['eenie', 'minie'];
     fakeApplyTheme = sinon.stub();
+    fakeIsThirdPartyUser = sinon.stub().returns(false);
     fakeSettings = {};
 
     fakeStore = {
+      defaultAuthority: sinon.stub(),
       getDraft: sinon.stub().returns(null),
+      getLink: sinon
+        .stub()
+        .callsFake((linkPath, { tag }) => `http://www.example.com/${tag}`),
     };
 
     $imports.$mock(mockImportedComponents());
     $imports.$mock({
+      '../../helpers/account-id': { isThirdPartyUser: fakeIsThirdPartyUser },
       '../../helpers/theme': { applyTheme: fakeApplyTheme },
       '../../store/use-store': { useStoreProxy: () => fakeStore },
     });
@@ -66,9 +73,25 @@ describe('AnnotationBody', () => {
     wrapper.update();
 
     const markdownView = wrapper.find('MarkdownView');
-    const tagList = wrapper.find('TagList');
+    const tagItems = wrapper.find('TagListItem');
     assert.strictEqual(markdownView.props().markdown, 'some text here');
-    assert.deepStrictEqual(tagList.props().tags, ['eenie', 'minie']);
+    assert.equal(tagItems.length, fakeAnnotation.tags.length);
+    assert.equal(tagItems.first().props().tag, 'eenie');
+  });
+
+  it('links the annotation tags', () => {
+    const wrapper = createBody();
+
+    const tagItems = wrapper.find('TagListItem');
+    assert.equal(tagItems.first().props().href, 'http://www.example.com/eenie');
+  });
+
+  it('does not link the annotation tags if the user is third party', () => {
+    fakeIsThirdPartyUser.returns(true);
+    const wrapper = createBody();
+
+    const tagItems = wrapper.find('TagListItem');
+    assert.isUndefined(tagItems.first().props().href);
   });
 
   it('renders the tags and text from the draft', () => {
@@ -78,9 +101,10 @@ describe('AnnotationBody', () => {
     wrapper.update();
 
     const markdownView = wrapper.find('MarkdownView');
-    const tagList = wrapper.find('TagList');
+    const tagItems = wrapper.find('TagListItem');
     assert.strictEqual(markdownView.props().markdown, 'this is a draft');
-    assert.deepStrictEqual(tagList.props().tags, ['1', '2']);
+    assert.equal(tagItems.length, 2);
+    assert.equal(tagItems.first().props().tag, '1');
   });
 
   it('does not render controls to expand/collapse the excerpt if it is not collapsible', () => {

src/sidebar/components/TagEditor.js

 import {
-  Icon,
   normalizeKeyName,
   useElementShouldClose,
 } from '@hypothesis/frontend-shared';
@@ -8,6 +7,8 @@ import { useRef, useState } from 'preact/hooks';
 import { withServices } from '../service-context';
 
 import AutocompleteList from './AutocompleteList';
+import TagList from './TagList';
+import TagListItem from './TagListItem';
 
 /** @typedef {import("preact").JSX.Element} JSXElement */
 
@@ -48,7 +49,7 @@ function TagEditor({
   });
 
   // Set up callback to monitor outside click events to close the AutocompleteList
-  const closeWrapperRef = /** @type {{ current: HTMLElement }} */ (useRef());
+  const closeWrapperRef = /** @type {{ current: HTMLDivElement }} */ (useRef());
   useElementShouldClose(closeWrapperRef, suggestionsListOpen, () => {
     setSuggestionsListOpen(false);
   });
@@ -269,36 +270,13 @@ function TagEditor({
     activeItem >= 0 ? `${tagEditorId}-AutocompleteList-item-${activeItem}` : '';
 
   return (
-    <div className="TagEditor">
-      <ul
-        className="TagEditor__tags"
-        aria-label="Suggested tags for annotation"
-      >
+    <div className="TagEditor space-y-4">
+      <TagList>
         {tagList.map(tag => {
-          return (
-            <li
-              key={`${tag}`}
-              className="TagEditor__item"
-              aria-label={`Tag: ${tag}`}
-            >
-              <span lang="" className="TagEditor__edit">
-                {tag}
-              </span>
-              <button
-                onClick={() => {
-                  onRemoveTag(tag);
-                }}
-                aria-label={`Remove Tag: ${tag}`}
-                title={`Remove Tag: ${tag}`}
-                className="TagEditor__delete"
-              >
-                <Icon name="cancel" />
-              </button>
-            </li>
-          );
+          return <TagListItem key={tag} onRemoveTag={onRemoveTag} tag={tag} />;
         })}
-      </ul>
-      <span
+      </TagList>
+      <div
         id={tagEditorId}
         className="TagEditor__combobox-wrapper"
         ref={closeWrapperRef}
@@ -332,7 +310,7 @@ function TagEditor({
           itemPrefixId={`${tagEditorId}-AutocompleteList-item-`}
           activeItem={activeItem}
         />
-      </span>
+      </div>
     </div>
   );
 }

src/sidebar/components/TagList.js

-import { useMemo } from 'preact/hooks';
-import { useStoreProxy } from '../store/use-store';
-
-import { isThirdPartyUser } from '../helpers/account-id';
-
-/** @typedef {import('../../types/api').Annotation} Annotation */
-
 /**
  * @typedef TagListProps
- * @prop {Annotation} annotation - Annotation that owns the tags.
- * @prop {string[]} tags - List of tags as strings.
+ * @prop {import("preact").ComponentChildren} children
  */
 
 /**
- * Component to render an annotation's tags.
+ * Render a list container for a list of annotation tags.
+ *
  * @param {TagListProps} props
  */
-function TagList({ annotation, tags }) {
-  const store = useStoreProxy();
-  const defaultAuthority = store.defaultAuthority();
-  const renderLink = useMemo(
-    // Show a link if the authority of the user is not 3rd party
-    () => !isThirdPartyUser(annotation.user, defaultAuthority),
-    [annotation, defaultAuthority]
-  );
-
-  /**
-   * Returns a uri link for a specific tag name.
-   * @param {string} tag
-   * @return {string}
-   */
-  const createTagSearchURL = tag => {
-    return store.getLink('search.tag', { tag });
-  };
-
+function TagList({ children }) {
   return (
-    <ul className="TagList" aria-label="Annotation tags">
-      {tags.map(tag => (
-        <li key={tag} className="TagList__item">
-          {renderLink && (
-            <a
-              className="TagList__link"
-              href={createTagSearchURL(tag)}
-              lang=""
-              target="_blank"
-              rel="noopener noreferrer"
-              aria-label={`Tag: ${tag}`}
-              title={`View annotations with tag: ${tag}`}
+    <ul
+      className="flex flex-wrap gap-2 leading-none"
+      aria-label="Annotation tags"
     >
-              {tag}
-            </a>
-          )}
-          {!renderLink && (
-            <span className="TagList__text" aria-label={`Tag: ${tag}`} lang="">
-              {tag}
-            </span>
-          )}
-        </li>
-      ))}
+      {children}
     </ul>
   );
 }

src/sidebar/components/TagListItem.js

+import { Icon, Link } from '@hypothesis/frontend-shared';
+import classnames from 'classnames';
+
+/**
+ * @typedef TagListItemProps
+ * @prop {string} [href] - If present, tag will be linked to this URL
+ * @prop {(tag: string) => void} [onRemoveTag] - Callback for deleting a tag. If
+ *   present, a delete button will be provided for the tag
+ * @prop {string} tag
+ */
+
+/**
+ * Render a single annotation tag as part of a list of tags
+ *
+ * @param {TagListItemProps} props
+ */
+export default function TagListItem({ href, onRemoveTag, tag }) {
+  return (
+    <li className="flex items-center border rounded-sm bg-grey-0">
+      <div className="grow px-1.5 py-1 touch:p-2">
+        {href ? (
+          <Link
+            classes="text-color-text-light hover:text-brand"
+            href={href}
+            lang=""
+            target="_blank"
+            aria-label={`Tag: ${tag}`}
+            title={`View annotations with tag: ${tag}`}
+          >
+            {tag}
+          </Link>
+        ) : (
+          <span
+            className="text-color-text-light cursor-default"
+            aria-label={`Tag: ${tag}`}
+            lang=""
+          >
+            {tag}
+          </span>
+        )}
+      </div>
+      {onRemoveTag && (
+        <button
+          className={classnames(
+            // More padding for mobile users to make touch target larger
+            'px-1.5 py-1 touch:p-2',
+            // Rounded border to match container edges and make keyboard focus
+            // ring shape conform. Turn off left side
+            // border radius to maintain a straight dividing line
+            'border-l rounded-sm rounded-l-none',
+            'text-grey-6 hover:text-color-text hover:bg-grey-2',
+            // Emulates transitions on *Button shared component styling
+            'transition-colors duration-200',
+            'hyp-u-outline-on-keyboard-focus--inset'
+          )}
+          onClick={() => {
+            onRemoveTag(tag);
+          }}
+          title={`Remove tag: ${tag}`}
+        >
+          <Icon classes="font-base" name="cancel" title={`Remove ${tag}`} />
+        </button>
+      )}
+    </li>
+  );
+}

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

@@ -390,16 +390,16 @@ describe('TagEditor', () => {
       });
     });
 
-    describe('when removing tags', () => {
-      it('removes `tag1` when clicking its delete button', () => {
-        const wrapper = createComponent(); // note: initial tagList is ['tag1', 'tag2']
-        assert.equal(wrapper.find('.TagEditor__edit').length, 2);
-        wrapper
-          .find('button')
-          .at(0) // delete 'tag1'
-          .simulate('click');
+    describe('tag removal', () => {
+      it('provides a tag removal callback to each tag in the tag list', () => {
+        const wrapper = createComponent();
+        const tagListItems = wrapper.find('TagListItem');
 
-        assert.calledWith(fakeOnRemoveTag, 'tag1');
+        assert.equal(tagListItems.length, 2);
+        assert.equal(
+          tagListItems.first().props().onRemoveTag,
+          wrapper.props().onRemoveTag
+        );
       });
     });
 

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

-import { mount } from 'enzyme';
-
-import TagList, { $imports } from '../TagList';
-
-import { checkAccessibility } from '../../../test-util/accessibility';
-import { mockImportedComponents } from '../../../test-util/mock-imported-components';
-
-describe('TagList', () => {
-  let fakeIsThirdPartyUser;
-  let fakeStore;
-  const fakeTags = ['tag1', 'tag2'];
-
-  function createComponent(props) {
-    return mount(<TagList annotation={{}} tags={fakeTags} {...props} />);
-  }
-
-  beforeEach(() => {
-    fakeIsThirdPartyUser = sinon.stub().returns(false);
-
-    fakeStore = {
-      defaultAuthority: sinon.stub().returns('hypothes.is'),
-      getLink: sinon.stub().returns('http://serviceurl.com'),
-    };
-
-    $imports.$mock(mockImportedComponents());
-    $imports.$mock({
-      '../helpers/account-id': {
-        isThirdPartyUser: fakeIsThirdPartyUser,
-      },
-      '../store/use-store': { useStoreProxy: () => fakeStore },
-    });
-  });
-
-  afterEach(() => {
-    $imports.$restore();
-  });
-
-  it('does not render any tags if `tags` prop is empty', () => {
-    const wrapper = createComponent({ tags: [] });
-    assert.isFalse(wrapper.find('.TagList__item a').exists());
-  });
-
-  context('when `isThirdPartyUser` returns false', () => {
-    it('adds appropriate classes, props and values', () => {
-      const wrapper = createComponent();
-      wrapper.find('a').forEach((link, i) => {
-        assert.isTrue(link.hasClass('TagList__link'));
-        assert.equal(link.prop('aria-label'), `Tag: ${fakeTags[i]}`);
-        assert.equal(link.prop('href'), 'http://serviceurl.com');
-        assert.equal(
-          link.prop('title'),
-          `View annotations with tag: ${fakeTags[i]}`
-        );
-        assert.equal(link.text(), fakeTags[i]);
-      });
-    });
-
-    it('gets the links for tags', () => {
-      createComponent();
-      assert.calledWith(fakeStore.getLink, 'search.tag', { tag: 'tag1' });
-      assert.calledWith(fakeStore.getLink, 'search.tag', { tag: 'tag2' });
-    });
-  });
-
-  context('when `isThirdPartyUser` returns true', () => {
-    beforeEach(() => {
-      fakeIsThirdPartyUser.returns(true);
-    });
-
-    it('adds appropriate classes, props and values', () => {
-      const wrapper = createComponent();
-      wrapper.find('span').forEach((link, i) => {
-        assert.isTrue(link.hasClass('TagList__text'));
-        assert.equal(link.prop('aria-label'), `Tag: ${fakeTags[i]}`);
-        assert.equal(link.text(), fakeTags[i]);
-      });
-    });
-
-    it('does not fetch tag link', () => {
-      createComponent();
-      assert.notCalled(fakeStore.getLink);
-    });
-  });
-
-  it(
-    'should pass a11y checks',
-    checkAccessibility([
-      {
-        name: 'first-party user',
-        content: () => createComponent({ tags: ['tag1', 'tag2'] }),
-      },
-      {
-        name: 'third-party user',
-        content: () => {
-          fakeIsThirdPartyUser.returns(true);
-          return createComponent({ tags: ['tag1', 'tag2'] });
-        },
-      },
-    ])
-  );
-});

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

+import { mount } from 'enzyme';
+
+import TagListItem from '../TagListItem';
+import TagList from '../TagList';
+
+import { checkAccessibility } from '../../../test-util/accessibility';
+
+describe('TagListItem', () => {
+  const createComponent = props =>
+    mount(<TagListItem tag="my tag" {...props} />);
+
+  // Render TagListItems in a list to appease semantic requirements of
+  // a11y tests
+  const createComponentInList = () =>
+    mount(
+      <TagList>
+        <TagListItem tag="my tag" href="http://www.example.com/my-tag" />
+        <TagListItem tag="purple" />
+      </TagList>
+    );
+
+  it('renders the tag text', () => {
+    const wrapper = createComponent();
+    assert.equal(wrapper.text(), 'my tag');
+  });
+
+  it('links the tag to the provided `href`', () => {
+    const wrapper = createComponent({ href: 'http:www.example.com/my-tag' });
+    const link = wrapper.find('Link');
+    assert.equal(link.props().href, 'http:www.example.com/my-tag');
+  });
+
+  it('renders a delete button if a removal callback is provided', () => {
+    const onRemoveTag = sinon.stub();
+    const wrapper = createComponent({ tag: 'banana', onRemoveTag });
+
+    assert.isTrue(wrapper.find('button[title="Remove tag: banana"]').exists());
+  });
+
+  it('invokes the removal callback when the delete button is clicked', () => {
+    const onRemoveTag = sinon.stub();
+    const wrapper = createComponent({ onRemoveTag });
+
+    wrapper.find('button').simulate('click');
+    assert.calledOnce(onRemoveTag);
+  });
+
+  it(
+    'should pass a11y checks',
+    checkAccessibility({
+      content: () => createComponentInList(),
+    })
+  );
+});

src/sidebar/icons.js

@@ -9,6 +9,7 @@ import {
   ccStd,
   ccZero,
   check,
+  collapse,
   copy,
   edit,
   editorLatex,
@@ -16,6 +17,7 @@ import {
   editorTextBold,
   editorTextItalic,
   email,
+  expand,
   external,
   flag,
   flagFilled,
@@ -67,12 +69,14 @@ export const sidebarIcons = {
   'cc-std': ccStd,
   'cc-zero': ccZero,
   'collapse-menu': caretUp,
+  collapse,
   collapsed: caretRight,
   copy,
   edit,
   email,
   'expand-menu': expandMenuIcon,
   error: cancel,
+  expand,
   external,
   facebook: socialFacebook,
   flag,

src/styles/sidebar/components/TagEditor.scss

 @use '../../mixins/forms';
-@use '../../mixins/buttons';
-@use '../../mixins/layout';
-@use '../../mixins/utils';
-@use '../../variables' as var;
 
 .TagEditor {
-  @include layout.vertical-rhythm(var.$layout-space--xsmall);
   &__input {
     @include forms.form-input;
     width: 100%;
   }
-
-  &__tags {
-    @include layout.row;
-    flex-wrap: wrap;
-  }
-
-  &__item {
-    @include layout.row;
-    margin: var.$layout-space--xxsmall var.$layout-space--xsmall
-      var.$layout-space--xsmall 0;
-  }
-
-  &__edit {
-    @include utils.border;
-    color: var.$color-text;
-    background: var.$grey-1;
-    border-radius: var.$border-radius 0 0 var.$border-radius;
-    border-right-width: 0;
-    padding: 2px var.$layout-space--xsmall;
-  }
-
-  &__delete {
-    @include buttons.button;
-    @include utils.border;
-    color: var.$grey-mid;
-    background-color: var.$grey-1;
-    padding: 0 var.$layout-space--xsmall;
-    border-radius: 0 var.$border-radius var.$border-radius 0;
-
-    &:hover {
-      background-color: var.$grey-2;
-    }
-  }
 }

src/styles/sidebar/components/TagList.scss

-@use '../../variables' as var;
-@use '../../mixins/layout';
-@use '../../mixins/utils';
-
-.TagList {
-  @include layout.row;
-  flex-wrap: wrap;
-
-  &__item {
-    @include utils.border;
-    margin: var.$layout-space--xxsmall var.$layout-space--xsmall
-      var.$layout-space--xxsmall 0;
-    padding: 2px var.$layout-space--xsmall;
-    background: var.$grey-0;
-    border-radius: var.$border-radius;
-  }
-
-  &__link,
-  &__text {
-    text-decoration: none;
-    color: var.$color-text--light;
-  }
-
-  &__link {
-    &:hover,
-    &:focus {
-      color: var.$color-brand;
-      border-color: var.$grey-4;
-    }
-  }
-}

src/styles/sidebar/components/_index.scss

@@ -33,7 +33,6 @@
 @use './SelectionTabs';
 @use './SearchInput';
 @use './TagEditor';
-@use './TagList';
 @use './Thread';
 @use './ThreadCard';
 @use './ThreadList';

tailwind.config.mjs

@@ -104,6 +104,7 @@ export default {
         },
       },
       screens: {
+        touch: { raw: '(pointer: coarse)' },
         md: '480px',
         // Narrow mobile screens
         'annotator-sm': '240px',