Merge pull request #1980 from hypothesis/collapsed-annotation-header

Tighten up display of headers for collapsed replies in threads

src/sidebar/components/annotation-header.js

@@ -2,6 +2,7 @@ import { createElement } from 'preact';
 import propTypes from 'prop-types';
 
 import { isHighlight, isReply } from '../util/annotation-metadata';
+import { isPrivate } from '../util/permissions';
 
 import AnnotationDocumentInfo from './annotation-document-info';
 import AnnotationShareInfo from './annotation-share-info';
@@ -23,18 +24,34 @@ export default function AnnotationHeader({
   showDocumentInfo,
   threadIsCollapsed,
 }) {
+  const isCollapsedReply = isReply(annotation) && threadIsCollapsed;
+  const annotationIsPrivate = isPrivate(
+    annotation.permissions,
+    annotation.user
+  );
   const annotationLink = annotation.links ? annotation.links.html : '';
-  const replyPluralized = !replyCount || replyCount > 1 ? 'replies' : 'reply';
+
   // NB: `created` and `updated` are strings, not `Date`s
   const hasBeenEdited =
     annotation.updated && annotation.created !== annotation.updated;
+  const showTimestamp = !isEditing;
+  const showEditedTimestamp = hasBeenEdited && !isCollapsedReply;
 
-  const showReplyButton = threadIsCollapsed && isReply(annotation);
+  const replyPluralized = replyCount > 1 ? 'replies' : 'reply';
   const replyButtonText = `${replyCount} ${replyPluralized}`;
+  const showReplyButton = replyCount > 0 && isCollapsedReply;
+  const showExtendedInfo = !isReply(annotation);
 
   return (
     <header className="annotation-header">
       <div className="annotation-header__row">
+        {annotationIsPrivate && (
+          <SvgIcon
+            className="annotation-header__icon"
+            name="lock"
+            title="This annotation is visible only to you"
+          />
+        )}
         <AnnotationUser annotation={annotation} />
         {showReplyButton && (
           <Button
@@ -44,9 +61,10 @@ export default function AnnotationHeader({
             title="Expand replies"
           />
         )}
-        {!isEditing && annotation.created && (
+
+        {showTimestamp && (
           <div className="annotation-header__timestamp">
-            {hasBeenEdited && (
+            {showEditedTimestamp && (
               <span className="annotation-header__timestamp-edited">
                 (edited{' '}
                 <Timestamp
@@ -67,20 +85,24 @@ export default function AnnotationHeader({
         )}
       </div>
 
-      <div className="annotation-header__row">
-        <AnnotationShareInfo annotation={annotation} />
-        {!isEditing && isHighlight(annotation) && (
-          <div className="annotation-header__highlight">
-            <SvgIcon
-              name="highlight"
-              title="This is a highlight. Click 'edit' to add a note or tag."
-              inline={true}
-              className="annotation-header__highlight-icon"
-            />
-          </div>
-        )}
-        {showDocumentInfo && <AnnotationDocumentInfo annotation={annotation} />}
-      </div>
+      {showExtendedInfo && (
+        <div className="annotation-header__row">
+          <AnnotationShareInfo annotation={annotation} />
+          {!isEditing && isHighlight(annotation) && (
+            <div className="annotation-header__highlight">
+              <SvgIcon
+                name="highlight"
+                title="This is a highlight. Click 'edit' to add a note or tag."
+                inline={true}
+                className="annotation-header__highlight-icon"
+              />
+            </div>
+          )}
+          {showDocumentInfo && (
+            <AnnotationDocumentInfo annotation={annotation} />
+          )}
+        </div>
+      )}
     </header>
   );
 }

src/sidebar/components/annotation-share-info.js

@@ -44,18 +44,9 @@ function AnnotationShareInfo({ annotation }) {
           </span>
         </a>
       )}
-      {annotationIsPrivate && (
+      {annotationIsPrivate && !linkToGroup && (
         <span className="annotation-share-info__private">
-          {/* Show the lock icon in all cases when the annotation is private... */}
-          <SvgIcon
-            className="annotation-share-info__icon"
-            name="lock"
-            title="This annotation is visible only to you"
-          />
-          {/* but only render the "Only Me" text if we're not showing/linking a group name */}
-          {!linkToGroup && (
-            <span className="annotation-share-info__private-info">Only me</span>
-          )}
+          <span className="annotation-share-info__private-info">Only me</span>
         </span>
       )}
     </div>

src/sidebar/components/annotation.js

@@ -4,7 +4,7 @@ import { useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
 import useStore from '../store/use-store';
-import { isNew, isReply, quote } from '../util/annotation-metadata';
+import { isReply, quote } from '../util/annotation-metadata';
 import { isShared } from '../util/permissions';
 import { withServices } from '../util/service-context';
 
@@ -38,6 +38,7 @@ function Annotation({
   const group = useStore(store => store.getGroup(annotation.group));
   const userid = useStore(store => store.profile().userid);
 
+  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;
@@ -51,7 +52,7 @@ function Annotation({
   const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
   const toggleText = `${toggleAction} (${replyCount})`;
 
-  const shouldShowActions = !isSaving && !isEditing && !isNew(annotation);
+  const shouldShowActions = !isSaving && !isEditing && !isCollapsedReply;
   const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
   const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
 

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

@@ -11,6 +11,7 @@ import mockImportedComponents from '../../../test-util/mock-imported-components'
 describe('AnnotationHeader', () => {
   let fakeIsHighlight;
   let fakeIsReply;
+  let fakeIsPrivate;
 
   const createAnnotationHeader = props => {
     return mount(
@@ -29,6 +30,7 @@ describe('AnnotationHeader', () => {
   beforeEach(() => {
     fakeIsHighlight = sinon.stub().returns(false);
     fakeIsReply = sinon.stub().returns(false);
+    fakeIsPrivate = sinon.stub();
 
     $imports.$mock(mockImportedComponents());
     $imports.$mock({
@@ -36,6 +38,9 @@ describe('AnnotationHeader', () => {
         isHighlight: fakeIsHighlight,
         isReply: fakeIsReply,
       },
+      '../util/permissions': {
+        isPrivate: fakeIsPrivate,
+      },
     });
   });
 
@@ -43,15 +48,33 @@ describe('AnnotationHeader', () => {
     $imports.$restore();
   });
 
-  describe('collapsed replies', () => {
+  describe('only me icon', () => {
+    it('should render an "Only Me" icon if the annotation is private', () => {
+      fakeIsPrivate.returns(true);
+
+      const wrapper = createAnnotationHeader();
+
+      assert.isTrue(wrapper.find('SvgIcon').filter({ name: 'lock' }).exists());
+    });
+    it('should not render an "Only Me" icon if the annotation is not private', () => {
+      fakeIsPrivate.returns(false);
+
+      const wrapper = createAnnotationHeader();
+
+      assert.isFalse(wrapper.find('SvgIcon').filter({ name: 'lock' }).exists());
+    });
+  });
+
+  describe('expand replies toggle button', () => {
     const findReplyButton = wrapper =>
       wrapper.find('Button').filter('.annotation-header__reply-toggle');
 
-    it('should render if annotation is a reply and thread is collapsed', () => {
+    it('should render if annotation is a collapsed reply and there are replies to show', () => {
       let fakeCallback = sinon.stub();
       fakeIsReply.returns(true);
       const wrapper = createAnnotationHeader({
         onReplyCountClick: fakeCallback,
+        replyCount: 1,
         threadIsCollapsed: true,
       });
 
@@ -60,6 +83,16 @@ describe('AnnotationHeader', () => {
       assert.equal(btn.props().onClick, fakeCallback);
     });
 
+    it('should not render if there are no replies to show', () => {
+      fakeIsReply.returns(true);
+      const wrapper = createAnnotationHeader({
+        threadIsCollapsed: true,
+        replyCount: 0,
+      });
+      const btn = findReplyButton(wrapper);
+      assert.isFalse(btn.exists());
+    });
+
     it('should not render if annotation is not a reply', () => {
       fakeIsReply.returns(false);
       const wrapper = createAnnotationHeader({
@@ -79,10 +112,6 @@ describe('AnnotationHeader', () => {
     });
 
     [
-      {
-        replyCount: 0,
-        expected: '0 replies',
-      },
       {
         replyCount: 1,
         expected: '1 reply',
@@ -112,15 +141,6 @@ describe('AnnotationHeader', () => {
       assert.isTrue(timestamp.exists());
     });
 
-    it('should not render timestamp container if annotation does not have a `created` value', () => {
-      const wrapper = createAnnotationHeader({
-        annotation: fixtures.newAnnotation(),
-      });
-      const timestamp = wrapper.find('.annotation-header__timestamp');
-
-      assert.isFalse(timestamp.exists());
-    });
-
     it('should render edited timestamp if annotation has been edited', () => {
       const annotation = fixtures.defaultAnnotation();
       annotation.updated = '2018-05-10T20:18:56.613388+00:00';
@@ -147,45 +167,74 @@ describe('AnnotationHeader', () => {
 
       assert.isFalse(timestamp.exists());
     });
-  });
 
-  describe('annotation is-highlight icon', () => {
-    it('should display is-highlight icon if annotation is a highlight', () => {
-      fakeIsHighlight.returns(true);
+    it('should not render edited timestamp if annotation is collapsed reply', () => {
+      const annotation = fixtures.defaultAnnotation();
+      annotation.updated = '2018-05-10T20:18:56.613388+00:00';
+      fakeIsReply.returns(true);
+
       const wrapper = createAnnotationHeader({
-        isEditing: false,
+        annotation: annotation,
+        threadIsCollapsed: true,
       });
-      const highlightIcon = wrapper.find('.annotation-header__highlight');
 
-      assert.isTrue(highlightIcon.exists());
+      const timestamp = wrapper
+        .find('Timestamp')
+        .filter('.annotation-header__timestamp-edited-link');
+
+      assert.isFalse(timestamp.exists());
     });
+  });
 
-    it('should not display the is-highlight icon if annotation is not a highlight', () => {
-      fakeIsHighlight.returns(false);
+  describe('extended header information', () => {
+    it('should not render extended header information if annotation is reply', () => {
+      fakeIsReply.returns(true);
       const wrapper = createAnnotationHeader({
-        isEditing: false,
+        showDocumentInfo: true,
       });
-      const highlightIcon = wrapper.find('.annotation-header__highlight');
 
-      assert.isFalse(highlightIcon.exists());
+      assert.isFalse(wrapper.find('AnnotationShareInfo').exists());
+      assert.isFalse(wrapper.find('AnnotationDocumentInfo').exists());
     });
-  });
 
-  describe('annotation document info', () => {
-    it('should render document info if `showDocumentInfo` is enabled', () => {
-      const wrapper = createAnnotationHeader({ showDocumentInfo: true });
+    describe('annotation is-highlight icon', () => {
+      it('should display is-highlight icon if annotation is a highlight', () => {
+        fakeIsHighlight.returns(true);
+        const wrapper = createAnnotationHeader({
+          isEditing: false,
+        });
+        const highlightIcon = wrapper.find('.annotation-header__highlight');
 
-      const documentInfo = wrapper.find('AnnotationDocumentInfo');
+        assert.isTrue(highlightIcon.exists());
+      });
+
+      it('should not display the is-highlight icon if annotation is not a highlight', () => {
+        fakeIsHighlight.returns(false);
+        const wrapper = createAnnotationHeader({
+          isEditing: false,
+        });
+        const highlightIcon = wrapper.find('.annotation-header__highlight');
 
-      assert.isTrue(documentInfo.exists());
+        assert.isFalse(highlightIcon.exists());
+      });
     });
 
-    it('should not render document info if `showDocumentInfo` is not enabled', () => {
-      const wrapper = createAnnotationHeader({ showDocumentInfo: false });
+    describe('annotation document info', () => {
+      it('should render document info if `showDocumentInfo` is enabled', () => {
+        const wrapper = createAnnotationHeader({ showDocumentInfo: true });
 
-      const documentInfo = wrapper.find('AnnotationDocumentInfo');
+        const documentInfo = wrapper.find('AnnotationDocumentInfo');
+
+        assert.isTrue(documentInfo.exists());
+      });
 
-      assert.isFalse(documentInfo.exists());
+      it('should not render document info if `showDocumentInfo` is not enabled', () => {
+        const wrapper = createAnnotationHeader({ showDocumentInfo: false });
+
+        const documentInfo = wrapper.find('AnnotationDocumentInfo');
+
+        assert.isFalse(documentInfo.exists());
+      });
     });
   });
 

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

@@ -106,30 +106,12 @@ describe('AnnotationShareInfo', () => {
 
       assert.notOk(privacy.exists());
     });
+
     context('private annotation', () => {
       beforeEach(() => {
         fakeIsPrivate.returns(true);
       });
 
-      it('should show privacy icon', () => {
-        const wrapper = createAnnotationShareInfo();
-
-        const privacyIcon = wrapper.find(
-          '.annotation-share-info__private .annotation-share-info__icon'
-        );
-
-        assert.isOk(privacyIcon.exists());
-        assert.equal(privacyIcon.prop('name'), 'lock');
-      });
-      it('should not show "only me" text for first-party group', () => {
-        const wrapper = createAnnotationShareInfo();
-
-        const privacyText = wrapper.find(
-          '.annotation-share-info__private-info'
-        );
-
-        assert.notOk(privacyText.exists());
-      });
       it('should show "only me" text for annotation in third-party group', () => {
         fakeGetGroup.returns({ name: 'Some Name' });
         const wrapper = createAnnotationShareInfo();

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

@@ -62,7 +62,6 @@ describe('Annotation', () => {
     };
 
     fakeMetadata = {
-      isNew: sinon.stub(),
       isReply: sinon.stub(),
       quote: sinon.stub(),
     };
@@ -463,10 +462,9 @@ describe('Annotation', () => {
       assert.isFalse(wrapper.find('AnnotationActionBar').exists());
     });
 
-    it('should not show annotation actions for new annotation', () => {
-      fakeMetadata.isNew.returns(true);
-
-      const wrapper = createComponent();
+    it('should not show annotations if the annotation is a collapsed reply', () => {
+      fakeMetadata.isReply.returns(true);
+      const wrapper = createComponent({ threadIsCollapsed: true });
 
       assert.isFalse(wrapper.find('AnnotationActionBar').exists());
     });

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

@@ -14,6 +14,13 @@
     align-items: baseline;
   }
 
+  &__icon {
+    margin-right: 5px;
+    width: 10px;
+    height: 10px;
+    color: var.$grey-6;
+  }
+
   &__reply-toggle.button {
     padding: 0 0.5em;
     font-weight: 400;