Merge pull request #1988 from hypothesis/annotation-layout

Fix annotation and thread vertical rhythm and layout

src/sidebar/components/annotation-body.js

-import { Fragment, createElement } from 'preact';
+import { createElement } from 'preact';
 import { useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
@@ -8,6 +8,8 @@ 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';
 
 /**
  * Display the rendered content of an annotation.
@@ -15,7 +17,9 @@ import MarkdownView from './markdown-view';
 export default function AnnotationBody({
   annotation,
   isEditing,
+  onEditTags,
   onEditText,
+  tags,
   text,
 }) {
   // Should the text content of `Excerpt` be rendered in a collapsed state,
@@ -31,10 +35,12 @@ export default function AnnotationBody({
     ? 'Show full annotation text'
     : 'Show the first few lines only';
 
+  const showExcerpt = !isEditing && text.length > 0;
+  const showTagList = !isEditing && tags.length > 0;
+
   return (
-    <Fragment>
     <section className="annotation-body">
-        {!isEditing && (
+      {showExcerpt && (
         <Excerpt
           collapse={isCollapsed}
           collapsedHeight={400}
@@ -60,7 +66,6 @@ export default function AnnotationBody({
           onEditText={onEditText}
         />
       )}
-      </section>
       {isCollapsible && !isEditing && (
         <div className="annotation-body__collapse-toggle">
           <Button
@@ -71,7 +76,9 @@ export default function AnnotationBody({
           />
         </div>
       )}
-    </Fragment>
+      {showTagList && <TagList annotation={annotation} tags={tags} />}
+      {isEditing && <TagEditor onEditTags={onEditTags} tagList={tags} />}
+    </section>
   );
 }
 
@@ -86,11 +93,18 @@ AnnotationBody.propTypes = {
    */
   isEditing: propTypes.bool,
 
+  /**
+   * Callback invoked when the user edits tags.
+   */
+  onEditTags: propTypes.func,
+
   /**
    * Callback invoked when the user edits the content of the annotation body.
    */
   onEditText: propTypes.func,
 
+  tags: propTypes.array.isRequired,
+
   /**
    * The markdown annotation body, which is either rendered as HTML (if `isEditing`
    * is false) or displayed in a text area otherwise.

src/sidebar/components/annotation.js

@@ -15,8 +15,6 @@ import AnnotationLicense from './annotation-license';
 import AnnotationPublishControl from './annotation-publish-control';
 import AnnotationQuote from './annotation-quote';
 import Button from './button';
-import TagEditor from './tag-editor';
-import TagList from './tag-list';
 
 /**
  * A single annotation.
@@ -52,7 +50,7 @@ function Annotation({
   const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
   const toggleText = `${toggleAction} (${replyCount})`;
 
-  const shouldShowActions = !isSaving && !isEditing && !isCollapsedReply;
+  const shouldShowActions = !isSaving && !isEditing;
   const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
   const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
 
@@ -108,15 +106,21 @@ function Annotation({
         showDocumentInfo={showDocumentInfo}
         threadIsCollapsed={threadIsCollapsed}
       />
+
       {hasQuote && <AnnotationQuote annotation={annotation} />}
+
+      {!isCollapsedReply && (
         <AnnotationBody
           annotation={annotation}
           isEditing={isEditing}
+          onEditTags={onEditTags}
           onEditText={onEditText}
+          tags={tags}
           text={text}
         />
-      {isEditing && <TagEditor onEditTags={onEditTags} tagList={tags} />}
-      {!isEditing && <TagList annotation={annotation} tags={tags} />}
+      )}
+
+      {!isCollapsedReply && (
         <footer className="annotation__footer">
           {isEditing && (
             <div className="annotation__form-actions">
@@ -139,11 +143,15 @@ function Annotation({
             {isSaving && <div className="annotation__actions">Saving...</div>}
             {shouldShowActions && (
               <div className="annotation__actions">
-              <AnnotationActionBar annotation={annotation} onReply={onReply} />
+                <AnnotationActionBar
+                  annotation={annotation}
+                  onReply={onReply}
+                />
               </div>
             )}
           </div>
         </footer>
+      )}
     </article>
   );
 }

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

@@ -16,6 +16,8 @@ describe('AnnotationBody', () => {
       <AnnotationBody
         annotation={fixtures.defaultAnnotation()}
         isEditing={false}
+        onEditTags={() => null}
+        tags={[]}
         text="test comment"
         {...props}
       />
@@ -88,10 +90,41 @@ describe('AnnotationBody', () => {
     assert.equal(buttonProps.title, 'Show the first few lines only');
   });
 
+  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'] });
+
+      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'] });
+
+      assert.isTrue(wrapper.find('TagEditor').exists());
+      assert.isFalse(wrapper.find('TagList').exists());
+    });
+  });
+
   it(
     'should pass a11y checks',
-    checkAccessibility({
+    checkAccessibility([
+      {
         content: () => createBody(),
-    })
+      },
+      {
+        name: 'when annotation has tags (tag list)',
+        content: () => createBody({ isEditing: false, tags: ['foo', 'bar'] }),
+      },
+      {
+        name: 'when annotation is being edited and has tags',
+        content: () => createBody({ isEditing: true, tags: ['foo', 'bar'] }),
+      },
+    ])
   );
 });

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

@@ -8,8 +8,6 @@ import { checkAccessibility } from '../../../test-util/accessibility';
 import mockImportedComponents from '../../../test-util/mock-imported-components';
 import { waitFor } from '../../../test-util/wait';
 
-// @TODO Note this import as `Annotation` for easier updating later
-
 import Annotation from '../annotation';
 import { $imports } from '../annotation';
 
@@ -145,38 +143,6 @@ describe('Annotation', () => {
     });
   });
 
-  describe('tags', () => {
-    it('renders tag editor if `isEditing', () => {
-      setEditingMode(true);
-
-      const wrapper = createComponent();
-
-      assert.isTrue(wrapper.find('TagEditor').exists());
-      assert.isFalse(wrapper.find('TagList').exists());
-    });
-
-    it('updates annotation draft if tags changed', () => {
-      setEditingMode(true);
-      const wrapper = createComponent();
-
-      wrapper
-        .find('TagEditor')
-        .props()
-        .onEditTags({ tags: ['uno', 'dos'] });
-
-      const call = fakeStore.createDraft.getCall(0);
-      assert.calledOnce(fakeStore.createDraft);
-      assert.sameMembers(call.args[1].tags, ['uno', 'dos']);
-    });
-
-    it('renders tag list if not `isEditing', () => {
-      const wrapper = createComponent();
-
-      assert.isTrue(wrapper.find('TagList').exists());
-      assert.isFalse(wrapper.find('TagEditor').exists());
-    });
-  });
-
   describe('publish control', () => {
     it('should show the publish control if in edit mode', () => {
       setEditingMode(true);
@@ -461,13 +427,38 @@ describe('Annotation', () => {
 
       assert.isFalse(wrapper.find('AnnotationActionBar').exists());
     });
+  });
 
-    it('should not show annotations if the annotation is a collapsed reply', () => {
+  context('annotation thread is collapsed', () => {
+    context('collapsed reply', () => {
+      beforeEach(() => {
         fakeMetadata.isReply.returns(true);
+      });
+
+      it('should not render body or footer', () => {
+        const wrapper = createComponent({ threadIsCollapsed: true });
+
+        assert.isFalse(wrapper.find('AnnotationBody').exists());
+        assert.isFalse(wrapper.find('footer').exists());
+      });
+
+      it('should not show actions', () => {
         const wrapper = createComponent({ threadIsCollapsed: true });
 
         assert.isFalse(wrapper.find('AnnotationActionBar').exists());
       });
+    });
+
+    context('collapsed top-level annotation', () => {
+      it('should render body and footer', () => {
+        fakeMetadata.isReply.returns(false);
+        const wrapper = createComponent({ threadIsCollapsed: true });
+
+        assert.isTrue(wrapper.find('AnnotationBody').exists());
+        assert.isTrue(wrapper.find('footer').exists());
+      });
+    });
+  });
 
   it(
     'should pass a11y checks',
@@ -482,7 +473,20 @@ describe('Annotation', () => {
           return createComponent();
         },
       },
+      {
+        name: 'when a collapsed top-level thread',
+        content: () => {
+          fakeMetadata.isReply.returns(false);
+          return createComponent({ threadIsCollapsed: true });
+        },
+      },
+      {
+        name: 'when a collapsed reply',
+        content: () => {
+          fakeMetadata.isReply.returns(true);
+          return createComponent({ threadIsCollapsed: true });
+        },
+      },
     ])
   );
-  });
 });

src/styles/sidebar/components/annotation-body.scss

@@ -3,12 +3,7 @@
 .annotation-body {
   @include var.font-normal;
   color: var.$grey-7;
-  // Margin between top of ascent of annotation body and
-  // bottom of ascent of annotation-quote should be ~15px.
-  margin-top: var.$layout-h-margin - 5px;
-  margin-bottom: var.$layout-h-margin;
-  margin-right: 0px;
-  margin-left: 0px;
+  margin: 1em 0;
 }
 
 .annotation-body__text {
@@ -38,12 +33,12 @@
 }
 
 .annotation-body__collapse-toggle {
-  // Negative top margin to bring this up tight under `.annotation-body`
-  margin-top: -(var.$layout-h-margin - 5px);
+  margin: 0.5em 0;
   display: flex;
   justify-content: flex-end;
 
   .annotation-body__collapse-toggle-button {
+    padding: 0.5em;
     background-color: transparent;
   }
 }

src/styles/sidebar/components/annotation-header.scss

@@ -3,9 +3,6 @@
 
 .annotation-header {
   @include forms.pie-clearfix;
-  // Margin between top of x-height of username and
-  // top of the annotation card should be ~15px
-  margin-top: -(var.$layout-h-margin) + 10px;
   color: var.$grey-6;
 
   &__row {

src/styles/sidebar/components/annotation-license.scss

@@ -4,7 +4,7 @@
   @include var.font-small;
   border-top: 1px solid var.$grey-3;
   padding-top: 0.5em;
-  margin: 0.5em 0;
+  margin: 1em 0;
 
   &__link {
     display: flex;

src/styles/sidebar/components/annotation-quote.scss

 @use '../../variables' as var;
 
 .annotation-quote {
-  // Margin between top of ascent of annotation quote and
-  // bottom of ascent of username should be ~15px
-  margin-top: var.$layout-h-margin - 5px;
-  // Margin between bottom of ascent of annotation quote and
-  // top of ascent of annotation-body should be ~15px
-  margin-bottom: var.$layout-h-margin - 3px;
+  margin: 1em 0;
 }
 
 .annotation-quote.is-orphan {

src/styles/sidebar/components/annotation.scss

@@ -66,42 +66,7 @@
   &__footer {
     @include var.font-normal;
     color: var.$grey-5;
-    margin-top: var.$layout-h-margin;
-  }
-
-  &--reply &__footer {
-    margin-top: var.$layout-h-margin - 8px;
-  }
-}
-
-// FIXME vertical rhythm here should be refactored
-.annotation--reply {
-  .annotation-header {
-    // Margin between bottom of ascent of annotation card footer labels
-    // and top of ascent of username should be ~20px
-    margin-top: 0px;
-  }
-
-  .annotation-body {
-    // Margin between top of ascent of annotation body and
-    // bottom of ascent of username should be ~15px
-    margin-top: var.$layout-h-margin - 8px;
-    // Margin between bottom of ascent of annotation body and
-    // top of annotation footer labels should be ~15px
-    margin-bottom: var.$layout-h-margin - 3px;
-  }
-}
-
-.annotation--reply.is-collapsed {
-  margin-bottom: 0;
-
-  .annotation-header {
-    margin: 0;
-  }
-
-  .annotation-body,
-  .annotation-footer {
-    display: none;
+    margin-top: 1em;
   }
 }
 

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

@@ -3,6 +3,8 @@
 @use "../../variables" as var;
 
 .tag-editor {
+  margin: 0.5em 0;
+
   &__input {
     @include forms.form-input;
     width: 100%;
@@ -11,12 +13,13 @@
   &__tags {
     display: flex;
     flex-wrap: wrap;
+    margin: 0.5em 0;
   }
 
   &__item {
     display: flex;
-    margin-right: 0.5em;
-    margin-bottom: 0.5em;
+    margin: 0.25em 0.5em 0.25em 0;
+    line-height: var.$base-line-height;
   }
 
   &__edit {

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

 @use "../../variables" as var;
 
 .tag-list {
+  margin: 1em 0;
   display: flex;
   flex-wrap: wrap;
 
   &__item {
-    display: flex;
-    margin-right: 0.5em;
-    margin-bottom: 0.5em;
+    margin: 0.25em 0.5em 0.25em 0;
+    padding: 0 0.5em;
+    line-height: var.$base-line-height;
+    background: var.$grey-1;
+    border: 1px solid var.$grey-3;
+    border-radius: 2px;
   }
 
   &__link,
   &__text {
     text-decoration: none;
     color: var.$grey-6;
-    background: var.$grey-1;
-    border: 1px solid var.$grey-3;
-    border-radius: 2px;
-    padding: 0 0.5em;
   }
 
   &__link {

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

@@ -11,7 +11,7 @@
   box-shadow: 0px 1px 1px 0px rgba(0, 0, 0, 0.1);
   border-radius: 2px;
   cursor: pointer;
-  padding: var.$layout-h-margin;
+  padding: 1em;
   background-color: var.$white;
 
   &:hover {

src/styles/sidebar/components/thread.scss

@@ -5,28 +5,35 @@
 
   &--reply {
     margin-top: 0.5em;
-    padding-top: 0.5em;
   }
 
+  // Conserve space for deeper usable nesting
+  &__children {
+    margin-left: -0.75em;
+  }
+
+  // Left "channel" of thread
   &__collapse {
-    margin: 0.25em;
-    margin-top: 0;
+    margin-right: 1em;
+    border-right: 1px dashed var.$grey-3;
+    // The entire channel is NOT clickable so don't make it look like it is
+    // (overrides `pointer` cursor applied to entire card)
     cursor: auto;
-    border-left: 1px dashed var.$grey-3;
 
+    // Darken thread line on hover as a visual cue to show related thread items
     &:hover {
-      border-left: 1px dashed var.$grey-4;
+      border-right: 1px dashed var.$grey-4;
     }
 
     .is-collapsed & {
-      border-left: none;
+      border-right: none;
     }
   }
 
   // TODO These styles should be consolidated with other `Button` styles
   &__collapse-button {
-    margin-left: -1.25em;
-    padding: 0.25em 0.75em 1em 0.75em;
+    margin-right: -1.25em;
+    padding: 0.25em 0.75em 0.75em 0.75em;
     // Need a non-transparent background so that the dashed border line
     // does not show through the button
     background-color: var.$white;