Remove editing logic from `AnnotationBody`, `Annotation`

Consolidate annotation-editing logic and sub-components within
`AnnotationEditor`. `AnnotationBody` is for rendering content, and
`Annotation` is relieved of managing editing-related callbacks.

These changes also add a new feature: When saving an annotation,
`AnnotationEditor` will check the contents of `TagEditor`'s input
element. If any text remains there that has not yet bet "committed"
as a tag, that text will be added as a tag. This is to prevent tags
accidentally getting "lost" on save.

Fixes https://github.com/hypothesis/product-backlog/issues/1122

src/sidebar/components/annotation-body.js

@@ -8,9 +8,7 @@ import { applyTheme } from '../util/theme';
 
 import Button from './button';
 import Excerpt from './excerpt';
-import MarkdownEditor from './markdown-editor';
 import MarkdownView from './markdown-view';
-import TagEditor from './tag-editor';
 import TagList from './tag-list';
 
 /**
@@ -21,13 +19,6 @@ import TagList from './tag-list';
 /**
  * @typedef AnnotationBodyProps
  * @prop {Annotation} annotation - The annotation in question
- * @prop {boolean} [isEditing] - Whether to display the body in edit mode (if true) or view mode.
- * @prop {(a: Object<'tags', string[]>) => void} [onEditTags] - Callback invoked when the user edits tags.
- * @prop {(a?: Object<'text', string>) => void} [onEditText] - Callback invoked when the user edits the content of the annotation body.
- * @prop {string[]} tags
- * @prop {string} text -
- *     The markdown annotation body, which is either rendered as HTML (if `isEditing`
- *     is false) or displayed in a text area otherwise.
  * @prop {MergedConfig} settings
  */
 
@@ -36,15 +27,7 @@ import TagList from './tag-list';
  *
  * @param {AnnotationBodyProps} props
  */
-function AnnotationBody({
-  annotation,
-  isEditing,
-  onEditTags,
-  onEditText,
-  tags,
-  text,
-  settings,
-}) {
+function AnnotationBody({ annotation, settings }) {
   // Should the text content of `Excerpt` be rendered in a collapsed state,
   // assuming it is collapsible (exceeds allotted collapsed space)?
   const [isCollapsed, setIsCollapsed] = useState(true);
@@ -54,8 +37,10 @@ function AnnotationBody({
   const [isCollapsible, setIsCollapsible] = useState(false);
 
   const toggleText = isCollapsed ? 'More' : 'Less';
-  const showExcerpt = !isEditing && text.length > 0;
-  const showTagList = !isEditing && tags.length > 0;
+  const tags = annotation.tags;
+  const text = annotation.text;
+  const showExcerpt = text.length > 0;
+  const showTagList = tags.length > 0;
 
   const textStyle = applyTheme(['annotationFontFamily'], settings);
 
@@ -80,15 +65,7 @@ function AnnotationBody({
           />
         </Excerpt>
       )}
-      {isEditing && (
-        <MarkdownEditor
-          textStyle={textStyle}
-          label="Annotation body"
-          text={text}
-          onEditText={onEditText}
-        />
-      )}
-      {isCollapsible && !isEditing && (
+      {isCollapsible && (
         <div className="annotation-body__collapse-toggle">
           {/* @ts-ignore - TODO: Button props need to be fixed */}
           <Button
@@ -104,18 +81,12 @@ function AnnotationBody({
         </div>
       )}
       {showTagList && <TagList annotation={annotation} tags={tags} />}
-      {isEditing && <TagEditor onEditTags={onEditTags} tagList={tags} />}
     </div>
   );
 }
 
 AnnotationBody.propTypes = {
   annotation: propTypes.object.isRequired,
-  isEditing: propTypes.bool,
-  onEditTags: propTypes.func,
-  onEditText: propTypes.func,
-  tags: propTypes.array.isRequired,
-  text: propTypes.string,
   settings: propTypes.object,
 };
 

src/sidebar/components/annotation.js

@@ -2,17 +2,14 @@ import classnames from 'classnames';
 import { createElement } from 'preact';
 import propTypes from 'prop-types';
 
-import { normalizeKeyName } from '../../shared/browser-compatibility-utils';
 import useStore from '../store/use-store';
 import { isReply, quote } from '../util/annotation-metadata';
-import { isShared } from '../util/permissions';
 import { withServices } from '../util/service-context';
 
 import AnnotationActionBar from './annotation-action-bar';
 import AnnotationBody from './annotation-body';
+import AnnotationEditor from './annotation-editor';
 import AnnotationHeader from './annotation-header';
-import AnnotationLicense from './annotation-license';
-import AnnotationPublishControl from './annotation-publish-control';
 import AnnotationQuote from './annotation-quote';
 import Button from './button';
 
@@ -28,7 +25,6 @@ import Button from './button';
  * @prop {boolean} showDocumentInfo - Should extended document info be rendered (e.g. in non-sidebar contexts)?
  * @prop {boolean} threadIsCollapsed - Is the thread to which this annotation belongs currently collapsed?
  * @prop {Object} annotationsService - Injected service
- * @prop {Object} toastMessenger - Injected service
  */
 
 /**
@@ -42,9 +38,7 @@ function Annotation({
   replyCount,
   showDocumentInfo,
   threadIsCollapsed,
-  toastMessenger,
 }) {
-  const createDraft = useStore(store => store.createDraft);
   const isFocused = useStore(store =>
     store.isAnnotationFocused(annotation.$tag)
   );
@@ -52,17 +46,12 @@ function Annotation({
 
   // An annotation will have a draft if it is being edited
   const draft = useStore(store => store.getDraft(annotation));
-  const group = useStore(store => store.getGroup(annotation.group));
   const userid = useStore(store => store.profile().userid);
   const isSaving = useStore(store => store.isSavingAnnotation(annotation));
 
   const isCollapsedReply = isReply(annotation) && threadIsCollapsed;
-  const isPrivate = draft ? draft.isPrivate : !isShared(annotation.permissions);
-  const tags = draft ? draft.tags : annotation.tags;
-  const text = draft ? draft.text : annotation.text;
 
   const hasQuote = !!quote(annotation);
-  const isEmpty = !text && !tags.length;
 
   const isEditing = !!draft && !isSaving;
 
@@ -70,55 +59,22 @@ function Annotation({
   const toggleText = `${toggleAction} (${replyCount})`;
 
   const shouldShowActions = !isSaving && !isEditing;
-  const shouldShowLicense =
-    isEditing && !isPrivate && group && group.type !== 'private';
   const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
 
-  const onEditTags = ({ tags }) => {
-    createDraft(annotation, { ...draft, tags });
-  };
-
-  const onEditText = ({ text }) => {
-    createDraft(annotation, { ...draft, text });
-  };
-
   const onReply = () => annotationsService.reply(annotation, userid);
 
-  const onSave = async () => {
-    try {
-      await annotationsService.save(annotation);
-    } catch (err) {
-      toastMessenger.error('Saving annotation failed');
-    }
-  };
-
-  // Allow saving of annotation by pressing CMD/CTRL-Enter
-  const onKeyDown = event => {
-    const key = normalizeKeyName(event.key);
-    if (isEmpty || !isEditing) {
-      return;
-    }
-    if ((event.metaKey || event.ctrlKey) && key === 'Enter') {
-      event.stopPropagation();
-      event.preventDefault();
-      onSave();
-    }
-  };
-
   const onToggleReplies = () =>
     // nb. We assume the annotation has an ID here because it is not possible
     // to create replies until the annotation has been saved.
     setExpanded(/** @type {string} */ (annotation.id), !!threadIsCollapsed);
 
   return (
-    /* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */
     <article
       className={classnames('annotation', {
         'annotation--reply': isReply(annotation),
         'is-collapsed': threadIsCollapsed,
         'is-focused': isFocused,
       })}
-      onKeyDown={onKeyDown}
     >
       <AnnotationHeader
         annotation={annotation}
@@ -132,27 +88,11 @@ function Annotation({
         <AnnotationQuote annotation={annotation} isFocused={isFocused} />
       )}
 
-      {!isCollapsedReply && (
-        <AnnotationBody
-          annotation={annotation}
-          isEditing={isEditing}
-          onEditTags={onEditTags}
-          onEditText={onEditText}
-          tags={tags}
-          text={text}
-        />
+      {!isCollapsedReply && !isEditing && (
+        <AnnotationBody annotation={annotation} />
       )}
 
-      {isEditing && (
-        <div className="annotation__form-actions u-layout-row">
-          <AnnotationPublishControl
-            annotation={annotation}
-            isDisabled={isEmpty}
-            onSave={onSave}
-          />
-        </div>
-      )}
-      {shouldShowLicense && <AnnotationLicense />}
+      {isEditing && <AnnotationEditor annotation={annotation} />}
 
       {!isCollapsedReply && (
         <footer className="annotation__footer">
@@ -186,9 +126,8 @@ Annotation.propTypes = {
   showDocumentInfo: propTypes.bool.isRequired,
   threadIsCollapsed: propTypes.bool.isRequired,
   annotationsService: propTypes.object.isRequired,
-  toastMessenger: propTypes.object.isRequired,
 };
 
-Annotation.injectedProps = ['annotationsService', 'toastMessenger'];
+Annotation.injectedProps = ['annotationsService'];
 
 export default withServices(Annotation);

src/sidebar/components/markdown-editor.js

@@ -447,7 +447,7 @@ export default function MarkdownEditor({
   };
 
   return (
-    <div>
+    <div className="markdown-editor">
       <Toolbar
         onCommand={handleCommand}
         isPreviewing={preview}

src/sidebar/components/tag-editor.js

@@ -17,9 +17,11 @@ let tagEditorIdCounter = 0;
 
 /**
  * @typedef TagEditorProps
- * @prop {(a: Object<'tags', string[]>) => any} onEditTags - Callback that saves the tag list.
- * @prop {string[]} tagList - The list of editable tags as strings.
- * @prop {Object} tags - Services
+ * @prop {(tag: string) => boolean} onAddTag - Callback to add a tag to the annotation
+ * @prop {(tag: string) => boolean} onRemoveTag - Callback to remove a tag from the annotation
+ * @prop {(tag: string) => any} onTagInput - Callback when inputted tag text changes
+ * @prop {string[]} tagList - The list of tags for the annotation under edit
+ * @prop {Object} tags - Injected service
  */
 
 /**
@@ -30,7 +32,13 @@ let tagEditorIdCounter = 0;
  *
  * @param {TagEditorProps} props
  */
-function TagEditor({ onEditTags, tags: tagsService, tagList }) {
+function TagEditor({
+  onAddTag,
+  onRemoveTag,
+  onTagInput,
+  tagList,
+  tags: tagsService,
+}) {
   const inputEl = useRef(/** @type {HTMLInputElement|null} */ (null));
   const [suggestions, setSuggestions] = useState(/** @type {string[]} */ ([]));
   const [activeItem, setActiveItem] = useState(-1); // -1 is unselected
@@ -49,6 +57,13 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
 
   const ie11 = useMemo(() => isIE11(), []);
 
+  /**
+   * Retrieve the current trimmed text value of the tag <input>
+   */
+  const pendingTag = () => inputEl.current.value.trim();
+  const hasPendingTag = () => pendingTag() && pendingTag().length > 0;
+  const clearPendingTag = () => (inputEl.current.value = '');
+
   /**
    * Helper function that returns a list of suggestions less any
    * results also found from the duplicates list.
@@ -72,13 +87,12 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
    * reset the activeItem and open the AutocompleteList
    */
   const updateSuggestions = () => {
-    const value = inputEl.current.value.trim();
-    if (value === '') {
+    if (!hasPendingTag()) {
       // If there is no input, just hide the suggestions
       setSuggestionsListOpen(false);
     } else {
       // Call filter() with a query value to return all matching suggestions.
-      const suggestions = tagsService.filter(value);
+      const suggestions = tagsService.filter(pendingTag());
       // Remove any repeated suggestions that are already tags
       // and set those to state.
       setSuggestions(removeDuplicates(suggestions, tagList));
@@ -88,51 +102,19 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
   };
 
   /**
-   * Handle changes to this annotation's tags
-   *
-   * @param {string[]} tagList
-   */
-  const updateTags = tagList => {
-    // update suggested tags list via service
-    tagsService.store(tagList.map(tag => ({ text: tag })));
-    onEditTags({ tags: tagList });
-  };
-
-  /**
-   * Remove a tag from this annotation.
-   *
-   * @param {string} tag
-   */
-  const removeTag = tag => {
-    const newTagList = [...tagList]; // make a copy
-    const index = newTagList.indexOf(tag);
-    newTagList.splice(index, 1);
-    updateTags(newTagList);
-  };
-
-  /**
-   * Adds a tag to the annotation equal to the value of the input field
-   * and then clears out the suggestions list and the input field.
+   * Invokes callback to add tag. If the tag was added, close the suggestions
+   * list, clear the field content and maintain focus.
    *
    * @param {string} newTag
    */
   const addTag = newTag => {
-    const value = newTag.trim();
-    if (value.length === 0) {
-      // don't add an empty tag
-      return;
-    }
-    if (tagList.indexOf(value) >= 0) {
-      // don't add duplicate tag
-      return;
-    }
-    updateTags([...tagList, value]);
+    if (onAddTag(newTag)) {
       setSuggestionsListOpen(false);
       setActiveItem(-1);
 
-    const input = inputEl.current;
-    input.value = '';
-    input.focus();
+      clearPendingTag();
+      inputEl.current.focus();
+    }
   };
 
   /**
@@ -141,6 +123,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
    * @param {import("preact").JSX.TargetedEvent<HTMLInputElement, InputEvent>} e
    */
   const handleOnInput = e => {
+    onTagInput?.(pendingTag());
     if (
       e.inputType === 'insertText' ||
       e.inputType === 'deleteContentBackward' ||
@@ -166,7 +149,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
    * Opens the AutocompleteList on focus if there is a value in the input
    */
   const handleFocus = () => {
-    if (inputEl.current.value.trim().length) {
+    if (hasPendingTag()) {
       setSuggestionsListOpen(true);
     }
   };
@@ -212,7 +195,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
         break;
       case 'Escape':
         // Clear any entered text, but retain focus
-        inputEl.current.value = '';
+        clearPendingTag();
         e.preventDefault();
         break;
       case 'Enter':
@@ -220,7 +203,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
         // Commit a tag
         if (activeItem === -1) {
           // nothing selected, just add the typed text
-          addTag(/** @type {HTMLInputElement} */ (inputEl.current).value);
+          addTag(pendingTag());
         } else {
           // Add the selected tag
           addTag(suggestions[activeItem]);
@@ -230,7 +213,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
       case 'Tab':
         // Commit a tag, or tab out of the field if it is empty (default browser
         // behavior)
-        if (inputEl.current.value.trim() === '') {
+        if (!hasPendingTag()) {
           // If the tag field is empty, allow `Tab` to have its default
           // behavior: continue to the next element in tab order
           break;
@@ -245,7 +228,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
           addTag(suggestions[0]);
         } else {
           // Commit the tag as typed in the field
-          addTag(/** @type {HTMLInputElement} */ (inputEl.current).value);
+          addTag(pendingTag());
         }
         // Retain focus
         e.preventDefault();
@@ -262,7 +245,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
    * @return {JSXElement} - Formatted tag for use in list
    */
   const formatSuggestItem = item => {
-    const curVal = inputEl.current.value.trim();
+    const curVal = pendingTag();
     const prefix = item.slice(0, item.indexOf(curVal));
     const suffix = item.slice(item.indexOf(curVal) + curVal.length);
 
@@ -283,7 +266,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
       : '';
 
   return (
-    <section className="tag-editor">
+    <div className="tag-editor">
       <ul
         className="tag-editor__tags"
         aria-label="Suggested tags for annotation"
@@ -300,7 +283,7 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
               </span>
               <button
                 onClick={() => {
-                  removeTag(tag);
+                  onRemoveTag(tag);
                 }}
                 aria-label={`Remove Tag: ${tag}`}
                 title={`Remove Tag: ${tag}`}
@@ -348,12 +331,14 @@ function TagEditor({ onEditTags, tags: tagsService, tagList }) {
           activeItem={activeItem}
         />
       </span>
-    </section>
+    </div>
   );
 }
 
 TagEditor.propTypes = {
-  onEditTags: propTypes.func.isRequired,
+  onAddTag: propTypes.func.isRequired,
+  onRemoveTag: propTypes.func.isRequired,
+  onTagInput: propTypes.func,
   tagList: propTypes.array.isRequired,
   tags: propTypes.object.isRequired,
 };

src/sidebar/components/test/annotation-body-test.js

@@ -11,17 +11,14 @@ import { checkAccessibility } from '../../../test-util/accessibility';
 import mockImportedComponents from '../../../test-util/mock-imported-components';
 
 describe('AnnotationBody', () => {
+  let fakeAnnotation;
   let fakeApplyTheme;
   let fakeSettings;
 
   function createBody(props = {}) {
     return mount(
       <AnnotationBody
-        annotation={fixtures.defaultAnnotation()}
-        isEditing={false}
-        onEditTags={() => null}
-        tags={[]}
-        text="test comment"
+        annotation={fakeAnnotation}
         settings={fakeSettings}
         {...props}
       />
@@ -29,6 +26,9 @@ describe('AnnotationBody', () => {
   }
 
   beforeEach(() => {
+    fakeAnnotation = fixtures.defaultAnnotation();
+    fakeAnnotation.text = 'some text here';
+    fakeAnnotation.tags = ['eenie', 'minie'];
     fakeApplyTheme = sinon.stub();
     fakeSettings = {};
 
@@ -42,24 +42,12 @@ describe('AnnotationBody', () => {
     $imports.$restore();
   });
 
-  it('displays the body if `isEditing` is false', () => {
-    const wrapper = createBody({ isEditing: false });
-    assert.isFalse(wrapper.exists('MarkdownEditor'));
-    assert.isTrue(wrapper.exists('MarkdownView'));
-  });
-
-  it('displays an editor if `isEditing` is true', () => {
-    const wrapper = createBody({ isEditing: true });
-    assert.isTrue(wrapper.exists('MarkdownEditor'));
-    assert.isFalse(wrapper.exists('MarkdownView'));
-  });
-
   it('does not render controls to expand/collapse the excerpt if it is not collapsible', () => {
     const wrapper = createBody();
 
     // By default, `isCollapsible` is `false` until changed by `Excerpt`,
     // so the expand/collapse button will not render
-    assert.notOk(wrapper.find('Button').exists());
+    assert.isFalse(wrapper.find('Button').exists());
   });
 
   it('renders controls to expand/collapse the excerpt if it is collapsible', () => {
@@ -109,25 +97,17 @@ describe('AnnotationBody', () => {
   });
 
   describe('tag list and editor', () => {
-    it('renders a list of tags if not editing and annotation has tags', () => {
-      const wrapper = createBody({ isEditing: false, tags: ['foo', 'bar'] });
+    it('renders a list of tags if annotation has tags', () => {
+      const wrapper = createBody();
 
       assert.isTrue(wrapper.find('TagList').exists());
     });
 
     it('does not render a tag list if annotation has no tags', () => {
-      const wrapper = createBody({ isEditing: false, tags: [] });
-
-      assert.isFalse(wrapper.find('TagList').exists());
-    });
-
-    it('renders a tag editor if annotation is being edited', () => {
-      const wrapper = createBody({ isEditing: true, tags: ['foo', 'bar'] });
+      const wrapper = createBody({ annotation: fixtures.defaultAnnotation() });
 
-      assert.isTrue(wrapper.find('TagEditor').exists());
       assert.isFalse(wrapper.find('TagList').exists());
     });
-  });
 
     it('applies theme', () => {
       const textStyle = { fontFamily: 'serif' };
@@ -136,14 +116,12 @@ describe('AnnotationBody', () => {
         .returns(textStyle);
 
       const wrapper = createBody();
-    assert.deepEqual(wrapper.find('MarkdownView').prop('textStyle'), textStyle);
-
-    wrapper.setProps({ isEditing: true });
       assert.deepEqual(
-      wrapper.find('MarkdownEditor').prop('textStyle'),
+        wrapper.find('MarkdownView').prop('textStyle'),
         textStyle
       );
     });
+  });
 
   it(
     'should pass a11y checks',
@@ -153,14 +131,14 @@ describe('AnnotationBody', () => {
       },
       {
         name: 'when annotation has tags (tag list)',
-        content: () => createBody({ isEditing: false, tags: ['foo', 'bar'] }),
+        content: () => {
+          const annotation = fixtures.defaultAnnotation();
+          annotation.tags = ['foo', 'bar'];
+          return createBody({ annotation });
         },
-      {
-        name: 'when annotation is being edited and has tags',
-        content: () => createBody({ isEditing: true, tags: ['foo', 'bar'] }),
       },
       {
-        name: 'when expandable and not editing',
+        name: 'when expandable',
         content: () => {
           const wrapper = createBody();
           act(() => {

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

@@ -6,7 +6,6 @@ import * as fixtures from '../../test/annotation-fixtures';
 
 import { checkAccessibility } from '../../../test-util/accessibility';
 import mockImportedComponents from '../../../test-util/mock-imported-components';
-import { waitFor } from '../../../test-util/wait';
 
 import Annotation from '../annotation';
 import { $imports } from '../annotation';
@@ -14,11 +13,9 @@ import { $imports } from '../annotation';
 describe('Annotation', () => {
   // Dependency Mocks
   let fakeMetadata;
-  let fakePermissions;
 
   // Injected dependency mocks
   let fakeAnnotationsService;
-  let fakeToastMessenger;
   let fakeStore;
 
   const setEditingMode = (isEditing = true) => {
@@ -35,7 +32,6 @@ describe('Annotation', () => {
       <Annotation
         annotation={fixtures.defaultAnnotation()}
         annotationsService={fakeAnnotationsService}
-        toastMessenger={fakeToastMessenger}
         replyCount={0}
         showDocumentInfo={false}
         threadIsCollapsed={true}
@@ -50,25 +46,13 @@ describe('Annotation', () => {
       save: sinon.stub().resolves(),
     };
 
-    fakeToastMessenger = {
-      error: sinon.stub(),
-    };
-
     fakeMetadata = {
       isReply: sinon.stub(),
       quote: sinon.stub(),
     };
 
-    fakePermissions = {
-      isShared: sinon.stub().returns(true),
-    };
-
     fakeStore = {
-      createDraft: sinon.stub(),
       getDraft: sinon.stub().returns(null),
-      getGroup: sinon.stub().returns({
-        type: 'private',
-      }),
       isAnnotationFocused: sinon.stub().returns(false),
       isSavingAnnotation: sinon.stub().returns(false),
       profile: sinon.stub().returns({ userid: 'acct:foo@bar.com' }),
@@ -78,7 +62,6 @@ describe('Annotation', () => {
     $imports.$mock(mockImportedComponents());
     $imports.$mock({
       '../util/annotation-metadata': fakeMetadata,
-      '../util/permissions': fakePermissions,
       '../store/use-store': callback => callback(fakeStore),
     });
   });
@@ -141,211 +124,12 @@ describe('Annotation', () => {
     });
   });
 
-  describe('annotation body and excerpt', () => {
-    it('updates annotation draft when text edited', () => {
-      const wrapper = createComponent();
-      const body = wrapper.find('AnnotationBody');
-
-      act(() => {
-        body.props().onEditText({ text: 'updated text' });
-      });
-
-      const call = fakeStore.createDraft.getCall(0);
-      assert.calledOnce(fakeStore.createDraft);
-      assert.equal(call.args[1].text, 'updated text');
-    });
-  });
-
-  describe('publish control', () => {
-    it('should show the publish control if in edit mode', () => {
-      setEditingMode(true);
-
-      const wrapper = createComponent();
-
-      assert.isTrue(wrapper.find('AnnotationPublishControl').exists());
-    });
-
-    it('should not show the publish control if not in edit mode', () => {
-      setEditingMode(false);
-
-      const wrapper = createComponent();
-
-      assert.isFalse(wrapper.find('AnnotationPublishControl').exists());
-    });
-
-    it('should enable the publish control if the annotation is not empty', () => {
-      const draft = fixtures.defaultDraft();
-      draft.text = 'bananas';
-      fakeStore.getDraft.returns(draft);
-
-      const wrapper = createComponent();
-
-      assert.isFalse(
-        wrapper.find('AnnotationPublishControl').props().isDisabled
-      );
-    });
-
-    it('should set the publish control to disabled if annotation is empty', () => {
-      const draft = fixtures.defaultDraft();
-      draft.tags = [];
-      draft.text = '';
-      fakeStore.getDraft.returns(draft);
-
-      const wrapper = createComponent();
-
-      assert.isTrue(
-        wrapper.find('AnnotationPublishControl').props().isDisabled
-      );
-    });
-
-    context('saving an annotation', () => {
-      it('should save the annotation when the publish control invokes the `onSave` callback', () => {
-        setEditingMode(true);
-
-        const wrapper = createComponent();
-        wrapper.find('AnnotationPublishControl').props().onSave();
-
-        assert.calledWith(
-          fakeAnnotationsService.save,
-          wrapper.props().annotation
-        );
-      });
-
   it('should show a "Saving" message when annotation is saving', () => {
-        setEditingMode(true);
     fakeStore.isSavingAnnotation.returns(true);
 
     const wrapper = createComponent();
 
-        assert.include(
-          wrapper.find('.annotation__actions').text(),
-          'Saving...'
-        );
-      });
-
-      it('should show an error message on failure', async () => {
-        setEditingMode(true);
-        fakeAnnotationsService.save.rejects();
-
-        const wrapper = createComponent();
-        wrapper.find('AnnotationPublishControl').props().onSave();
-
-        await waitFor(() => fakeToastMessenger.error.called);
-      });
-
-      describe('saving using shortcut-key combo', () => {
-        context('in editing mode with text or tag content populated', () => {
-          beforeEach(() => {
-            // Put into editing mode by presence of draft, and add some `text`
-            // so that the annotation is not seen as "empty"
-            const draft = fixtures.defaultDraft();
-            draft.text = 'bananas';
-            fakeStore.getDraft.returns(draft);
-          });
-
-          it('should save annotation if `CTRL+Enter` is typed', () => {
-            const wrapper = createComponent();
-
-            wrapper
-              .find('.annotation')
-              .simulate('keydown', { key: 'Enter', ctrlKey: true });
-
-            assert.calledWith(
-              fakeAnnotationsService.save,
-              wrapper.props().annotation
-            );
-          });
-
-          it('should save annotation if `META+Enter` is typed', () => {
-            const wrapper = createComponent();
-
-            wrapper
-              .find('.annotation')
-              .simulate('keydown', { key: 'Enter', metaKey: true });
-
-            assert.calledWith(
-              fakeAnnotationsService.save,
-              wrapper.props().annotation
-            );
-          });
-
-          it('should not save annotation if `META+g` is typed', () => {
-            // i.e. don't save on non-"Enter" keys
-            const wrapper = createComponent();
-
-            wrapper
-              .find('.annotation')
-              .simulate('keydown', { key: 'g', metaKey: true });
-
-            assert.notCalled(fakeAnnotationsService.save);
-          });
-        });
-
-        context('empty or not in editing mode', () => {
-          it('should not save annotation if not in editing mode', () => {
-            fakeStore.getDraft.returns(null);
-            const wrapper = createComponent();
-
-            wrapper
-              .find('.annotation')
-              .simulate('keydown', { key: 'Enter', metaKey: true });
-
-            assert.notCalled(fakeAnnotationsService.save);
-          });
-
-          it('should not save annotation if content is empty', () => {
-            fakeStore.getDraft.returns(fixtures.defaultDraft());
-            const wrapper = createComponent();
-
-            wrapper
-              .find('.annotation')
-              .simulate('keydown', { key: 'Enter', ctrlKey: true });
-
-            assert.notCalled(fakeAnnotationsService.save);
-          });
-        });
-      });
-    });
-  });
-
-  describe('license information', () => {
-    it('should show license information when editing shared annotations in public groups', () => {
-      fakeStore.getGroup.returns({ type: 'open' });
-      setEditingMode(true);
-
-      const wrapper = createComponent();
-
-      assert.isTrue(wrapper.find('AnnotationLicense').exists());
-    });
-
-    it('should not show license information when not editing', () => {
-      fakeStore.getGroup.returns({ type: 'open' });
-      setEditingMode(false);
-
-      const wrapper = createComponent();
-
-      assert.isFalse(wrapper.find('AnnotationLicense').exists());
-    });
-
-    it('should not show license information for annotations in private groups', () => {
-      fakeStore.getGroup.returns({ type: 'private' });
-      setEditingMode(true);
-
-      const wrapper = createComponent();
-
-      assert.isFalse(wrapper.find('AnnotationLicense').exists());
-    });
-
-    it('should not show license information for private annotations', () => {
-      const draft = fixtures.defaultDraft();
-      draft.isPrivate = true;
-      fakeStore.getGroup.returns({ type: 'open' });
-      fakeStore.getDraft.returns(draft);
-
-      const wrapper = createComponent();
-
-      assert.isFalse(wrapper.find('AnnotationLicense').exists());
-    });
+    assert.include(wrapper.find('.annotation__actions').text(), 'Saving...');
   });
 
   describe('reply thread toggle button', () => {

src/sidebar/components/test/tag-editor-test.js

@@ -14,7 +14,9 @@ describe('TagEditor', function () {
   let fakeTags = ['tag1', 'tag2'];
   let fakeTagsService;
   let fakeServiceUrl;
-  let fakeOnEditTags;
+  let fakeOnAddTag;
+  let fakeOnRemoveTag;
+  let fakeOnTagInput;
   let fakeIsIE11;
 
   function createComponent(props) {
@@ -26,7 +28,9 @@ describe('TagEditor', function () {
     return mount(
       <TagEditor
         // props
-        onEditTags={fakeOnEditTags}
+        onAddTag={fakeOnAddTag}
+        onRemoveTag={fakeOnRemoveTag}
+        onTagInput={fakeOnTagInput}
         tagList={fakeTags}
         // service props
         serviceUrl={fakeServiceUrl}
@@ -38,12 +42,13 @@ describe('TagEditor', function () {
   }
 
   beforeEach(function () {
-    fakeOnEditTags = sinon.stub();
+    fakeOnAddTag = sinon.stub().returns(true);
+    fakeOnRemoveTag = sinon.stub();
+    fakeOnTagInput = sinon.stub();
     fakeServiceUrl = sinon.stub().returns('http://serviceurl.com');
     fakeIsIE11 = sinon.stub().returns(false);
     fakeTagsService = {
       filter: sinon.stub().returns(['tag4', 'tag3']),
-      store: sinon.stub(),
     };
     $imports.$mock(mockImportedComponents());
     $imports.$mock({
@@ -205,14 +210,10 @@ describe('TagEditor', function () {
     /**
      * Helper function to assert that a tag was correctly added
      */
-    const assertAddTagsSuccess = (wrapper, tagList) => {
-      // saves the suggested tags to the service
-      assert.calledWith(
-        fakeTagsService.store,
-        tagList.map(tag => ({ text: tag }))
-      );
-      // called the onEditTags callback prop
-      assert.isTrue(fakeOnEditTags.calledWith({ tags: tagList }));
+    const assertAddTagsSuccess = (wrapper, tag) => {
+      // called the onAddTags callback
+      assert.calledOnce(fakeOnAddTag);
+      assert.calledWith(fakeOnAddTag, tag);
       // hides the suggestions
       assert.equal(wrapper.find('AutocompleteList').prop('open'), false);
       // removes the selected index
@@ -222,18 +223,11 @@ describe('TagEditor', function () {
       // input element should have focus
       assert.equal(document.activeElement.nodeName, 'INPUT');
     };
-    /**
-     * Helper function to assert that a tag was correctly not added
-     */
-    const assertAddTagsFail = () => {
-      assert.isTrue(fakeTagsService.store.notCalled);
-      assert.isTrue(fakeOnEditTags.notCalled);
-    };
 
     it('adds a tag from the <input> field', () => {
       const wrapper = createComponent();
       selectOption(wrapper, 'tag3');
-      assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 'tag3']);
+      assertAddTagsSuccess(wrapper, 'tag3');
     });
 
     [
@@ -246,7 +240,7 @@ describe('TagEditor', function () {
         wrapper.find('input').instance().value = 'umbrella';
         typeInput(wrapper); // opens suggestion list
         keyAction[0](wrapper);
-        assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 'umbrella']);
+        assertAddTagsSuccess(wrapper, 'umbrella');
         // ensure focus is still on the input field
         assert.equal(document.activeElement.nodeName, 'INPUT');
       });
@@ -264,7 +258,7 @@ describe('TagEditor', function () {
         // suggestions: [tag3, tag4]
         navigateDown(wrapper);
         keyAction[0](wrapper);
-        assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 'tag3']);
+        assertAddTagsSuccess(wrapper, 'tag3');
         // ensure focus is still on the input field
         assert.equal(document.activeElement.nodeName, 'INPUT');
       });
@@ -288,25 +282,19 @@ describe('TagEditor', function () {
     });
 
     context('When using the "Enter" key', () => {
-      it('should not add a tag if the <input> is empty', () => {
+      it('should invoke addTag callback even if input is empty', () => {
         const wrapper = createComponent();
         wrapper.find('input').instance().value = '';
         selectOptionViaEnter(wrapper);
-        assertAddTagsFail();
+        assertAddTagsSuccess(wrapper, '');
       });
 
-      it('should not add a tag if the <input> value is only white space', () => {
+      it('should invoke addTag callback even if <input> value is only white space', () => {
         const wrapper = createComponent();
         wrapper.find('input').instance().value = '  ';
         selectOptionViaEnter(wrapper);
-        assertAddTagsFail();
-      });
-
-      it('should not add a tag if its a duplicate of one already in the list', () => {
-        const wrapper = createComponent();
-        wrapper.find('input').instance().value = 'tag1';
-        selectOptionViaEnter(wrapper);
-        assertAddTagsFail();
+        // Callback will be invoked with the _trimmed_ string
+        assertAddTagsSuccess(wrapper, '');
       });
     });
 
@@ -317,7 +305,7 @@ describe('TagEditor', function () {
         wrapper.find('input').instance().value = 'tag33';
         typeInput(wrapper);
         selectOptionViaTab(wrapper);
-        assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 'tag33']);
+        assertAddTagsSuccess(wrapper, 'tag33');
         // ensure focus is still on the input field
         assert.equal(document.activeElement.nodeName, 'INPUT');
       });
@@ -328,7 +316,7 @@ describe('TagEditor', function () {
         wrapper.find('input').instance().value = 't';
         typeInput(wrapper);
         selectOptionViaTab(wrapper);
-        assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 't']);
+        assertAddTagsSuccess(wrapper, 't');
         // ensure focus is still on the input field
         assert.equal(document.activeElement.nodeName, 'INPUT');
       });
@@ -340,7 +328,7 @@ describe('TagEditor', function () {
         typeInput(wrapper);
         // suggestions: [tag3]
         selectOptionViaTab(wrapper);
-        assertAddTagsSuccess(wrapper, ['tag1', 'tag2', 'tag3']);
+        assertAddTagsSuccess(wrapper, 'tag3');
         // ensure focus is still on the input field
         assert.equal(document.activeElement.nodeName, 'INPUT');
       });
@@ -354,7 +342,6 @@ describe('TagEditor', function () {
         assert.equal(document.activeElement.nodeName, 'BODY');
       });
     });
-  });
 
     describe('when removing tags', () => {
       it('removes `tag1` when clicking its delete button', () => {
@@ -365,10 +352,7 @@ describe('TagEditor', function () {
           .at(0) // delete 'tag1'
           .simulate('click');
 
-      // saves the suggested tags to the service (only 'tag2' should be passed)
-      assert.isTrue(fakeTagsService.store.calledWith([{ text: 'tag2' }]));
-      // called the onEditTags callback prop  (only 'tag2' should be passed)
-      assert.isTrue(fakeOnEditTags.calledWith({ tags: ['tag2'] }));
+        assert.calledWith(fakeOnRemoveTag, 'tag1');
       });
     });
 
@@ -382,6 +366,7 @@ describe('TagEditor', function () {
           assert.equal(wrapper.find('AutocompleteList').prop('open'), true);
           assert.equal(wrapper.find('AutocompleteList').prop('activeItem'), -1);
         });
+
         it('should increment the `activeItem` when pressing down circularly', () => {
           fakeIsIE11.returns(ie11);
           const wrapper = createComponent();
@@ -410,6 +395,7 @@ describe('TagEditor', function () {
           navigateUp(wrapper);
           assert.equal(wrapper.find('AutocompleteList').prop('activeItem'), -1);
         });
+
         it('should set `activeItem` to -1 when clearing the suggestions', () => {
           fakeIsIE11.returns(ie11);
           const wrapper = createComponent();
@@ -425,6 +411,7 @@ describe('TagEditor', function () {
         });
       });
     });
+  });
 
   describe('accessibility attributes and ids', () => {
     it('creates multiple <TagEditor> components with unique autocomplete-list `id` props', () => {

src/styles/sidebar/components/markdown-editor.scss

@@ -4,6 +4,13 @@
 @use "../../mixins/utils";
 @use "../../variables" as var;
 
+.markdown-editor {
+  /* Reset line-height to avoid having extra gaps/vertical spacing in the
+     element's container
+  */
+  line-height: 1;
+}
+
 .markdown-editor__toolbar {
   @include layout.row;
   // Toolbar buttons wrap on non-touch devices if they don't fit. We don't use

src/styles/sidebar/components/tag-editor.scss

@@ -5,8 +5,7 @@
 @use "../../variables" as var;
 
 .tag-editor {
-  margin: var.$layout-space--xsmall 0;
-
+  @include layout.vertical-rhythm(var.$layout-space--xsmall);
   &__input {
     @include forms.form-input;
     width: 100%;
@@ -15,7 +14,6 @@
   &__tags {
     @include layout.row;
     flex-wrap: wrap;
-    margin: var.$layout-space--xsmall 0;
   }
 
   &__item {

src/styles/sidebar/components/tag-list.scss

@@ -5,7 +5,6 @@
 .tag-list {
   @include layout.row;
   flex-wrap: wrap;
-  margin: 1em 0;
 
   &__item {
     @include utils.border;