Merge pull request #2966 from hypothesis/annotation-element-directive

Make annotation directive more consistent with other directives

h/static/scripts/directive/annotation.js

@@ -254,11 +254,6 @@ function AnnotationController(
     /** Whether or not this annotation is private. */
     vm.isPrivate = false;
 
-    /** Copy isSidebar from $scope onto vm for consistency (we want this
-      * directive's templates to always access variables from vm rather than
-      * directly from scope). */
-    vm.isSidebar = $scope.isSidebar;
-
     /** A fuzzy, relative (eg. '6 days ago') format of the annotation's
      * last update timestamp
      */
@@ -290,7 +285,7 @@ function AnnotationController(
       * haven't been saved yet - the data that will be saved to the server when
       * they are saved).
       */
-    domainModel = $scope.annotationGet();
+    domainModel = vm.annotation;
 
     /**
       * `true` if this AnnotationController instance was created as a result of
@@ -811,17 +806,18 @@ function link(scope, elem, attrs, controllers) {
 // @ngInject
 function annotation($document) {
   return {
+    restrict: 'E',
+    bindToController: true,
     controller: AnnotationController,
     controllerAs: 'vm',
     link: link,
     require: ['annotation', '?^thread', '?^threadFilter', '?^deepCount'],
     scope: {
-      annotationGet: '&annotation',
+      annotation: '=',
       // Indicates whether this is the last reply in a thread.
       isLastReply: '=',
-      replyCount: '@annotationReplyCount',
-      replyCountClick: '&annotationReplyCountClick',
-      showReplyCount: '@annotationShowReplyCount',
+      replyCount: '=',
+      onReplyCountClick: '&',
       isSidebar: '='
     },
     templateUrl: 'annotation.html'

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

@@ -4,6 +4,7 @@
 var Promise = require('core-js/library/es6/promise');
 
 var events = require('../../events');
+var util = require('./util');
 
 var module = angular.mock.module;
 var inject = angular.mock.inject;
@@ -423,17 +424,14 @@ describe('annotation', function() {
 
     function createDirective(annotation) {
       annotation = annotation || defaultAnnotation();
-      $scope.annotation = annotation;
-      var element = angular.element('<div annotation="annotation">');
-      compileService()(element)($scope);
-      $scope.$digest();
-      var controller = element.controller('annotation');
-      var scope = element.isolateScope();
+      var element = util.createDirective(document, 'annotation', {
+        annotation: annotation,
+      });
       return {
         annotation: annotation,
-        controller: controller,
+        controller: element.ctrl,
         element: element,
-        scope: scope
+        scope: element.scope,
       };
     }
 
@@ -948,11 +946,12 @@ describe('annotation', function() {
       });
 
       it('makes the annotation shared if the parent is shared', function() {
-        var controller = createDirective(annotation).controller;
-        $scope.annotation.group = 'my group';
-        $scope.annotation.permissions = {
-          read: ['my group']
+        var annotation = defaultAnnotation();
+        annotation.group = 'my group';
+        annotation.permissions = {
+          read: ['my-group'],
         };
+        var controller = createDirective(annotation).controller;
         var reply = {};
         fakeAnnotationMapper.createAnnotation.returns(reply);
         fakePermissions.isShared = function(permissions, group) {
@@ -982,12 +981,13 @@ describe('annotation', function() {
       );
 
       it('sets the reply\'s group to be the same as its parent\'s', function() {
+        var annotation = defaultAnnotation();
+        annotation.group = 'my group';
         var controller = createDirective(annotation).controller;
-        $scope.annotation.group = 'my group';
         var reply = {};
         fakeAnnotationMapper.createAnnotation.returns(reply);
         controller.reply();
-        assert.equal(reply.group, $scope.annotation.group);
+        assert.equal(reply.group, annotation.group);
       });
     });
 
@@ -1719,7 +1719,7 @@ describe('annotation', function() {
         $provide.value('documentDomainFilter', locals.documentDomainFilter);
         $provide.value('localStorage', locals.localStorage);
       });
-      locals.element = angular.element('<div annotation="annotation">');
+      locals.element = angular.element('<annotation annotation="annotation">');
       var compiledElement = compileService()(locals.element);
       locals.$rootScope = getRootScope();
       locals.parentScope = locals.$rootScope.$new();

h/static/scripts/directive/test/thread-test.coffee

@@ -215,39 +215,6 @@ describe 'thread', ->
           count.withArgs('match').returns(1)
           assert.isTrue(controller.shouldShowAsReply())
 
-    describe '#shouldShowNumReplies', ->
-      count = null
-      filterActive = false
-
-      beforeEach ->
-        createDirective()
-        count = sinon.stub()
-        controller.counter = {count: count}
-        controller.filter = {active: -> filterActive}
-
-      describe 'when not filtered', ->
-        it 'shows the reply if the thread has children', ->
-          count.withArgs('message').returns(1)
-          assert.isTrue(controller.shouldShowNumReplies())
-
-        it 'does not show the reply if the thread has no children', ->
-          count.withArgs('message').returns(0)
-          assert.isFalse(controller.shouldShowNumReplies())
-
-      describe 'when filtered with children', ->
-        beforeEach ->
-          filterActive = true
-
-        it 'shows the reply', ->
-          count.withArgs('match').returns(1)
-          count.withArgs('message').returns(1)
-          assert.isTrue(controller.shouldShowNumReplies())
-
-        it 'does not show the reply if the message count does not match the match count', ->
-          count.withArgs('match').returns(0)
-          count.withArgs('message').returns(1)
-          assert.isFalse(controller.shouldShowNumReplies())
-
     describe '#numReplies', ->
 
       beforeEach ->

h/static/scripts/directive/test/util.js

@@ -11,7 +11,7 @@ function hyphenate(name) {
  * A helper for instantiating an AngularJS directive in a unit test.
  *
  * Usage:
- *   var domElement = createDirective('myComponent', {
+ *   var domElement = createDirective(document, 'myComponent', {
  *     attrA: 'initial-value'
  *   }, {
  *     scopePropery: scopeValue
@@ -25,7 +25,7 @@ function hyphenate(name) {
  *
  * The initial value may be a callback function to invoke. eg:
  *
- * var domElement = createDirective('myComponent', {
+ * var domElement = createDirective(document, 'myComponent', {
  *  onEvent: function () {
  *    console.log('event triggered');
  *  }
@@ -34,7 +34,7 @@ function hyphenate(name) {
  * If the callback accepts named arguments, these need to be specified
  * via an object with 'args' and 'callback' properties:
  *
- * var domElement = createDirective('myComponent', {
+ * var domElement = createDirective(document, 'myComponent', {
  *   onEvent: {
  *     args: ['arg1'],
  *     callback: function (arg1) {

h/static/scripts/directive/thread.coffee

@@ -80,19 +80,6 @@ ThreadController = [
       else
         return shouldShowUnfiltered
 
-    ###*
-    # @ngdoc method
-    # @name thread.ThreadController#shouldShowNumReplies
-    # @description
-    # Returns a boolean indicating whether the reply count should be rendered
-    # for the annotation at the root of this thread.
-    ###
-    this.shouldShowNumReplies = ->
-      hasChildren = this._count('message') > 0
-      allRepliesShown = this._count('message') == this._count('match')
-      hasFilterMatch = !this._isFilterActive() || allRepliesShown
-      hasChildren && hasFilterMatch
-
     ###*
     # @ngdoc method
     # @name thread.ThreadController#numReplies

h/templates/client/annotation.html

@@ -15,8 +15,8 @@
 
       <span class="annotation-collapsed-replies">
         <a class="annotation-link" href=""
-          ng-click="replyCountClick()"
-          ng-pluralize count="replyCount"
+          ng-click="vm.onReplyCountClick()"
+          ng-pluralize count="vm.replyCount"
           when="{'0': '', 'one': '1 reply', 'other': '{} replies'}"></a>
       </span>
 
@@ -140,10 +140,10 @@
       </a>
     </div>
 
-    <div class="annotation-replies" ng-if="replyCount > 0">
+    <div class="annotation-replies" ng-if="vm.replyCount > 0">
       <a class="annotation-link" href=""
-         ng-click="replyCountClick()"
-         ng-pluralize count="replyCount"
+         ng-click="vm.onReplyCountClick()"
+         ng-pluralize count="vm.replyCount"
          when="{'0': '', 'one': '1 reply', 'other': '{} replies'}"></a>
     </div>
 

h/templates/client/thread.html

@@ -11,17 +11,16 @@
   <p><em>Message not available.</em></p>
 </div>
 
-<article class="annotation thread-message {{vm.collapsed && 'collapsed'}}"
+<annotation class="annotation thread-message {{vm.collapsed && 'collapsed'}}"
          name="annotation"
          annotation="vm.container.message"
          is-last-reply="$last"
          is-sidebar="::isSidebar"
-         annotation-show-reply-count="{{vm.shouldShowNumReplies()}}"
-         annotation-reply-count="{{vm.numReplies()}}"
-         annotation-reply-count-click="vm.toggleCollapsed()"
+         reply-count="vm.numReplies()"
+         on-reply-count-click="vm.toggleCollapsed()"
          ng-if="vm.container.message"
          ng-show="vm.matchesFilter()">
-</article>
+</annotation>
 
 <div class="thread-load-more" ng-show="vm.shouldShowLoadMore()">
   <a class="load-more small"