Implement new design for hovered conversation threads (#3376)

* Implement new design for hovered conversation threads

Implement the new design for hovered replies from
https://trello.com/c/aXCXxzx2 .

The most visible effect is that conversation threads have a grey
background when hovered.

In the process of implementing the new styling, there is some cleanup
of the CSS:

 * Use `--reply`/`--top-reply` modifier classes on <annotation> and
   <annotation-thread> elements to style annotations, top-level replies
   and nested replies differently. This makes the CSS simpler and
   reduces the risk of unexpected side effects that come with descendant
   selectors.

 * Rename `thread` CSS classes to match the name of the component
   that they are used in, `annotation-thread`.

* Move 'annotation-unavailable-message' styling to app.scss

This class is used in the root template (viewer.html) not the
<annotation-thread> component.

* Darken expand/collapse toggle arrow only when annotation itself is hovered

Darken the expand/collapse arrow when an annotation is hovered but not
when its replies are hovered.

* Remove no-op CSS class

The `clear: both` styling had no effect because <annotation-thread>
is now using flexbox rather than floats for layout.

* Do not show 'Hide replies' link for replies

For replies there were two different ways to collapse the annotation
card, the expand/collapse toggle arrow and the 'Hide replies' link.

Removing the 'Hide replies' link avoids having two ways to do the same
thing and makes the cards look cleaner.

* Remove the light grey background for hovered replies

Following design review, remove the grey background for hovered replies.

* Make rendering of dashed lines to the left of replies better in Chrome

Previously the dashed line started at the top of the <annotation-thread>
component and the top part was covered up by the thread expand/collapse
toggle.

In Chrome the alignment of dashes within a dashed border varies as the
height of the element changes [1]. Therefore depending on the height of
the reply, this could result in the visible part of the line below the
collapse/expand toggle starting at either a gap or dash in the line.

By instead moving the dashed line to a separate element which is
positioned beneath the expand/collapse toggle, the first visible dash in
the reply line always appears in the same place and is aligned correctly
with the annotation content to its right.

[1] See http://www.impressivewebs.com/comparison-css-border-style/
    for a visual representation of why this is done.

h/static/scripts/build-thread.js

@@ -191,12 +191,16 @@ function sortThread(thread, compareFn, replyCompareFn) {
 }
 
 /**
- * Return a copy of @p thread with the replyCount property updated.
+ * Return a copy of @p thread with the `replyCount` and `depth` properties
+ * updated.
  */
-function countReplies(thread) {
-  var children = thread.children.map(countReplies);
+function countRepliesAndDepth(thread, depth) {
+  var children = thread.children.map(function (c) {
+    return countRepliesAndDepth(c, depth + 1);
+  });
   return Object.assign({}, thread, {
     children: children,
+    depth: depth,
     replyCount: children.reduce(function (total, child) {
       return total + 1 + child.replyCount;
     }, 0),
@@ -324,8 +328,8 @@ function buildThread(annotations, opts) {
   // Sort the root thread according to the current search criteria
   thread = sortThread(thread, opts.sortCompareFn, opts.replySortCompareFn);
 
-  // Update reply counts
-  thread = countReplies(thread);
+  // Update `replyCount` and `depth` properties
+  thread = countRepliesAndDepth(thread, -1);
 
   return thread;
 }

h/static/scripts/directive/annotation-thread.js

@@ -30,6 +30,10 @@ function showAllParents(thread, showFn) {
 
 // @ngInject
 function AnnotationThreadController() {
+  // Flag that tracks whether the content of the annotation is hovered,
+  // excluding any replies.
+  this.annotationHovered = false;
+
   this.toggleCollapsed = function () {
     this.onChangeCollapsed({
       id: this.thread.id,
@@ -37,6 +41,30 @@ function AnnotationThreadController() {
     });
   };
 
+  this.threadClasses = function () {
+    return {
+      'annotation-thread': true,
+      'annotation-thread--reply': this.thread.depth > 0,
+      'annotation-thread--top-reply': this.thread.depth === 1,
+    };
+  };
+
+  this.threadToggleClasses = function () {
+    return {
+      'annotation-thread__collapse-toggle': true,
+      'is-open': !this.thread.collapsed,
+      'is-hovered': this.annotationHovered,
+    };
+  };
+
+  this.annotationClasses = function () {
+    return {
+      annotation: true,
+      'annotation--reply': this.thread.depth > 0,
+      'is-collapsed': this.thread.collapsed,
+    };
+  };
+
   /**
    * Show this thread and any of its children
    */
@@ -46,6 +74,10 @@ function AnnotationThreadController() {
     showAllChildren(this.thread, this.onForceVisible);
   };
 
+  this.isTopLevelThread = function () {
+    return !this.thread.parent;
+  };
+
   /**
    * Return the total number of annotations in the current
    * thread which have been hidden because they do not match the current

h/static/scripts/directive/annotation.js

@@ -690,6 +690,10 @@ function AnnotationController(
     return persona.username(domainModel.user);
   };
 
+  vm.isReply = function () {
+    return isReply(domainModel);
+  };
+
   /**
    * Sets whether or not the controls for expanding/collapsing the body of
    * lengthy annotations should be shown.

h/static/scripts/directive/test/annotation-thread-test.js

@@ -10,10 +10,12 @@ function PageObject(element) {
     return Array.from(element[0].querySelectorAll('annotation'));
   };
   this.visibleReplies = function () {
-    return Array.from(element[0].querySelectorAll('.thread:not(.ng-hide)'));
+    return Array.from(element[0].querySelectorAll(
+      '.annotation-thread__content > ul > li:not(.ng-hide)'
+    ));
   };
   this.replyList = function () {
-    return element[0].querySelector('.thread-replies');
+    return element[0].querySelector('.annotation-thread__content > ul');
   };
   this.isHidden = function (element) {
     return element.classList.contains('ng-hide');

h/static/scripts/test/build-thread-test.js

@@ -327,4 +327,16 @@ describe('build-thread', function () {
       }]);
     });
   });
+
+  describe('depth', function () {
+    it('is 0 for annotations', function () {
+      var thread = createThread(SIMPLE_FIXTURE, {}, ['depth']);
+      assert.deepEqual(thread[0].depth, 0);
+    });
+
+    it('is 1 for top-level replies', function () {
+      var thread = createThread(SIMPLE_FIXTURE, {}, ['depth']);
+      assert.deepEqual(thread[0].children[0].depth, 1);
+    });
+  });
 });

h/static/styles/annotation-thread.scss

+.annotation-thread {
+  display: flex;
+  flex-direction: row;
+}
+
+// Direct or nested reply to an annotation
+.annotation-thread--reply {
+  // Left margin is set so that left edge of collapse toggle arrow
+  // for the reply is aligned with the left edge of the parent annotation's
+  // content.
+  margin-left: -5px;
+}
+
+// Top-level reply to an annotation
+.annotation-thread--top-reply {
+  padding-top: 5px;
+  padding-bottom: 5px;
+}
+
+li:first-child .annotation-thread--top-reply {
+  // Gap between baseline of 'Hide/Show Replies' for annotation and top
+  // of first reply should be ~15px
+  margin-top: 5px;
+}
+
+// Container for the toggle arrow and dashed line at the left edge of replies.
+.annotation-thread__thread-edge {
+  display: flex;
+  flex-direction: column;
+  width: 8px;
+  margin-right: 13px;
+}
+
+// The dashed line at the left edge of replies
+.annotation-thread__thread-line {
+  border-right: 1px dashed $grey-3;
+  flex-grow: 1;
+}
+
+.annotation-thread__content {
+  flex-grow: 1;
+}
+
+// Darken expand/collapse toggle when an annotation is hovered. This is only
+// when the annotation itself is hovered, not the replies.
+.annotation-thread__collapse-toggle:hover,
+.annotation-thread__collapse-toggle.is-hovered {
+  color: $grey-7;
+}
+
+// Toggle arrow which expands and collapses threads.
+// This is aligned so that it appears above a dashed line which appears
+// to the left of the threads.
+.annotation-thread__collapse-toggle {
+  width: 10px;
+  color: $grey-4;
+  display: block;
+  text-align: center;
+
+  font-size: 15px;
+  line-height: 22px;
+
+  height: 100%;
+
+  &.is-open {
+    // When the thread is expanded, the top of the dashed line is should be
+    // aligned with the top of the privacy indicator ("Only me") if present
+    height: 24px;
+  }
+}

h/static/styles/annotation.scss

@@ -10,7 +10,7 @@ $annotation-card-left-padding: 10px;
   box-shadow: 0px 1px 1px 0px rgba(0, 0, 0, 0.10);
   border-radius: 2px;
   padding: $layout-h-margin;
-  background: $white;
+  background-color: $white;
 }
 
 .annotation-card:hover {
@@ -211,22 +211,20 @@ $annotation-card-left-padding: 10px;
   display: none;
 }
 
-.thread-replies {
-  .annotation.collapsed {
-    margin-bottom: 0;
+.annotation--reply.is-collapsed {
+  margin-bottom: 0;
 
-    .annotation-header {
-      margin: 0;
-    }
+  .annotation-header {
+    margin: 0;
+  }
 
-    .annotation-body, .annotation-footer {
-      display: none;
-    }
+  .annotation-body, .annotation-footer {
+    display: none;
+  }
 
-    .annotation-collapsed-replies {
-      display: inline;
-      margin-left: .25em;
-    }
+  .annotation-collapsed-replies {
+    display: inline;
+    margin-left: .25em;
   }
 }
 
@@ -241,3 +239,31 @@ $annotation-card-left-padding: 10px;
   flex-direction: row;
   margin-bottom: 10px;
 }
+
+// Style adjustments for annotation cards that are replies
+.annotation--reply {
+  .annotation-action-btn {
+    color: $grey-4;
+  }
+
+  .annotation-footer {
+    // Margin between bottom of ascent of annotation body and
+    // top of annotation footer should be ~15px
+    margin-top: $layout-h-margin - 8px;
+  }
+
+  .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: $layout-h-margin - 8px;
+    // Margin between bottom of ascent of annotation body and
+    // top of annotation footer labels should be ~15px
+    margin-bottom: $layout-h-margin - 3px;
+  }
+}

h/static/styles/app.scss

@@ -11,6 +11,7 @@ $base-line-height: 20px;
 @import './about-this-version-dialog';
 @import './annotation';
 @import './annotation-share-dialog';
+@import './annotation-thread';
 @import './api-token-input';
 @import './dropdown-menu-btn';
 @import './excerpt';
@@ -26,7 +27,6 @@ $base-line-height: 20px;
 @import './simple-search';
 @import './spinner';
 @import './tags-input';
-@import './thread';
 @import './tooltip';
 @import './top-bar';
 
@@ -63,6 +63,9 @@ body {
   }
 }
 
+// Elements in root template (viewer.html)
+// ---------------------------------------
+
 .create-account-banner {
   background-color: $gray-dark;
   border-radius: 2px;
@@ -104,3 +107,38 @@ body {
     top: 1em;
   }
 }
+
+.thread-list {
+  & > * {
+    // Default spacing between items in the annotation card list
+    margin-bottom: .72em;
+  }
+}
+
+.thread-list__spacer {
+  // This is a hidden element which is used to reserve space for off-screen
+  // threads, so it should not occupy any space other than that set via its
+  // 'height' inline style property.
+  margin: 0;
+}
+
+.annotation-unavailable-message {
+  display: flex;
+  flex-direction: column;
+  border: 1px solid $gray-lighter;
+  padding-top: 30px;
+  padding-bottom: 30px;
+  border-radius: 3px;
+  align-items: center;
+
+  &__label {
+    text-align: center;
+  }
+
+  &__icon {
+    background-image: url(../images/icons/lock.svg);
+    background-repeat: no-repeat;
+    width: 56px;
+    height: 48px;
+  }
+}

h/static/styles/markdown.scss

@@ -9,6 +9,7 @@
 }
 
 .markdown-tools {
+  background-color: $white;
   border-top: .1em solid #D3D3D3;
   border-left: .1em solid #D3D3D3;
   border-right: .1em solid #D3D3D3;

h/static/styles/thread.scss

-$thread-padding: $annotation-card-left-padding;
-
-.thread-list {
-  & > * {
-    margin-bottom: .72em;
-  }
-
-  & > li.thread > .threadexp {
-    display: none;
-  }
-}
-
-.thread-list__spacer {
-  // This is a hidden element which is used to reserve space for off-screen
-  // threads, so it should not occupy any space other than that set via its
-  // 'height' inline style property.
-  margin: 0;
-}
-
-.annotation-unavailable-message {
-  display: flex;
-  flex-direction: column;
-  border: 1px solid $gray-lighter;
-  padding-top: 30px;
-  padding-bottom: 30px;
-  border-radius: 3px;
-  align-items: center;
-
-  &__label {
-    text-align: center;
-  }
-
-  &__icon {
-    background-image: url(../images/icons/lock.svg);
-    background-repeat: no-repeat;
-    width: 56px;
-    height: 48px;
-  }
-}
-
-.thread-replies {
-  .annotation-action-btn {
-    color: $grey-4;
-  }
-
-  .annotation-footer {
-    // Margin between bottom of ascent of annotation body and
-    // top of annotation footer should be ~15px
-    margin-top: $layout-h-margin - 8px;
-    // Margin between bottom of ascent of annotation footer labels
-    // and top of ascent of username should be ~20px
-    margin-bottom: $layout-h-margin - 3px;
-  }
-
-  .annotation-header {
-    // Margin between bottom of ascent of annotation card footer labels
-    // and top of ascent of username should be ~20px
-    margin-top: $layout-h-margin - 2px;
-  }
-
-  .annotation-body {
-    // Margin between top of ascent of annotation body and
-    // bottom of ascent of username should be ~15px
-    margin-top: $layout-h-margin - 8px;
-    // Margin between bottom of ascent of annotation body and
-    // top of annotation footer labels should be ~15px
-    margin-bottom: $layout-h-margin - 3px;
-  }
-}
-
-.thread {
-  cursor: pointer;
-  position: relative;
-
-  & > ul {
-    padding-left: $thread-padding + 3px;
-    padding-right: $thread-padding + 3px;
-    margin-left: -$thread-padding;
-    margin-right: -$thread-padding;
-  }
-
-  // nested threads for annotation replies
-  .thread {
-    border-left: 1px dotted $grey-3;
-    padding: 0;
-    padding-left: $thread-padding;
-
-    &:hover {
-      & > .threadexp > span {
-        color: $grey-7;
-      }
-    }
-  }
-
-  .threadexp {
-    position: absolute;
-    left: -.7em;
-    width: 1.4em;
-    height: 1.4em;
-    font-size: 1.1em;
-
-    span {
-      background: $white;
-      color: $grey-4;
-      display: block;
-      line-height: inherit;
-      text-align: center;
-    }
-  }
-}
-
-.thread-load-more {
-  clear: both;
-}

h/templates/client/annotation.html

@@ -140,7 +140,7 @@
       </a>
     </div>
 
-    <div class="annotation-replies" ng-if="vm.replyCount > 0">
+    <div class="annotation-replies" ng-if="!vm.isReply() && vm.replyCount > 0">
       <a href=""
         ng-click="vm.onReplyCountClick()">
         <span class="annotation-replies__link">{{ vm.isCollapsed ? 'Show replies' : 'Hide replies' }}</span>

h/templates/client/annotation_thread.html

-<a href=""
-   class="threadexp"
-   title="{{vm.thread.collapsed && 'Expand' || 'Collapse'}}"
-   ng-click="vm.toggleCollapsed()"
-   ng-if="vm.thread.parent">
-   <span ng-class="{'h-icon-arrow-right': vm.thread.collapsed,
-                    'h-icon-arrow-drop-down': !vm.thread.collapsed}"></span>
-</a>
+<div ng-class="vm.threadClasses()">
+  <div class="annotation-thread__thread-edge" ng-if="!vm.isTopLevelThread()">
+    <a href=""
+       ng-class="vm.threadToggleClasses()"
+       title="{{vm.thread.collapsed && 'Expand' || 'Collapse'}}"
+       ng-click="vm.toggleCollapsed()">
+       <span ng-class="{'h-icon-arrow-right': vm.thread.collapsed,
+                        'h-icon-arrow-drop-down': !vm.thread.collapsed}"></span>
+    </a>
+    <div class="annotation-thread__thread-line"></div>
+  </div>
+  <div class="annotation-thread__content">
+    <annotation ng-class="vm.annotationClasses()"
+             annotation="vm.thread.annotation"
+             is-collapsed="vm.thread.collapsed"
+             is-last-reply="$last"
+             is-sidebar="::vm.isSidebar"
+             name="annotation"
+             ng-mouseenter="vm.annotationHovered = true"
+             ng-mouseleave="vm.annotationHovered = false"
+             ng-if="vm.thread.annotation"
+             ng-show="vm.thread.visible"
+             show-document-info="vm.showDocumentInfo"
+             on-reply-count-click="vm.toggleCollapsed()"
+             reply-count="vm.thread.replyCount">
+    </annotation>
 
-<!-- Annotation -->
-<annotation class="annotation thread-message {{vm.thread.collapsed && 'collapsed'}}"
-         annotation="vm.thread.annotation"
-         is-collapsed="vm.thread.collapsed"
-         is-last-reply="$last"
-         is-sidebar="::vm.isSidebar"
-         name="annotation"
-         ng-if="vm.thread.annotation"
-         ng-show="vm.thread.visible"
-         show-document-info="vm.showDocumentInfo"
-         on-reply-count-click="vm.toggleCollapsed()"
-         reply-count="vm.thread.replyCount">
-</annotation>
+    <div ng-if="!vm.thread.annotation" class="thread-deleted">
+      <p><em>Message not available.</em></p>
+    </div>
 
-<div ng-if="!vm.thread.annotation" class="thread-deleted">
-  <p><em>Message not available.</em></p>
-</div>
+    <div ng-if="vm.hiddenCount() > 0">
+      <a class="small"
+         href=""
+         ng-click="vm.showThreadAndReplies()"
+         ng-pluralize
+         count="vm.hiddenCount()"
+         when="{'0': '',
+               one: 'View one more in conversation',
+               other: 'View {} more in conversation'}"
+         ></a>
+    </div>
 
-<div class="thread-load-more" ng-if="vm.hiddenCount() > 0">
-  <a class="load-more small"
-     href=""
-     ng-click="vm.showThreadAndReplies()"
-     ng-pluralize
-     count="vm.hiddenCount()"
-     when="{'0': '',
-           one: 'View one more in conversation',
-           other: 'View {} more in conversation'}"
-     ></a>
+    <!-- Replies -->
+    <ul ng-show="!vm.thread.collapsed">
+      <li ng-repeat="child in vm.thread.children track by child.id"
+          ng-show="vm.shouldShowReply(child)">
+        <annotation-thread
+          show-document-info="false"
+          thread="child"
+          on-change-collapsed="vm.onChangeCollapsed({id:id, collapsed:collapsed})"
+          on-force-visible="vm.onForceVisible({thread:thread})">
+        </annotation-thread>
+      </li>
+    </ul>
+  </div>
 </div>
-
-<!-- Replies -->
-<ul class="thread-replies" ng-show="!vm.thread.collapsed">
-  <li class="thread"
-      ng-repeat="child in vm.thread.children track by child.id"
-      ng-show="vm.shouldShowReply(child)">
-    <annotation-thread
-      show-document-info="false"
-      thread="child"
-      on-change-collapsed="vm.onChangeCollapsed({id:id, collapsed:collapsed})"
-      on-force-visible="vm.onForceVisible({thread:thread})">
-    </annotation-thread>
-  </li>
-</ul>

h/templates/client/viewer.html

@@ -35,7 +35,7 @@
   <li class="thread-list__spacer"
       ng-style="{height: virtualThreadList.offscreenUpperHeight}"></li>
   <li id="{{child.id}}"
-      class="annotation-card thread"
+      class="annotation-card"
       ng-class="{'js-hover': hasFocus(child.annotation)}"
       ng-mouseenter="focus(child.annotation)"
       ng-click="scrollTo(child.annotation)"