Merge pull request #1662 from hypothesis/1620-multiple-down-arrows-tweaks

Tweaks to Remove bad css classes from buckets

h/static/scripts/plugin/heatmap.coffee

@@ -315,6 +315,7 @@ class Annotator.Plugin.Heatmap extends Annotator.Plugin
       div.addClass('heatmap-pointer')
 
       # Creates highlights corresponding bucket when mouse is hovered
+      # TODO: This should use event delegation on the container.
       .on 'mousemove', (event) =>
         bucket = @tabs.index(event.currentTarget)
         for hl in @annotator.getHighlights()
@@ -351,32 +352,34 @@ class Annotator.Plugin.Heatmap extends Annotator.Plugin
           annotator.selectAnnotations annotations,
             (event.ctrlKey or event.metaKey),
 
-    @tabs
-    .attr 'title', (d) =>
-      if (len = @buckets[d].length) > 1
-        "Show #{len} annotations"
-      else if len > 0
-        "Show one annotation"
+    this._buildTabs(@tabs, @buckets)
 
-    .css 'margin-top', (d) =>
-      if @isUpper(d) or @isLower(d) then '-9px' else '-8px'
+    if @dynamicBucket
+      @annotator.updateViewer this._getDynamicBucket()
 
-    .css 'top', (d) =>
-      "#{(@index[d] + @index[d+1]) / 2}px"
+  _buildTabs: ->
+    @tabs.each (d, el) =>
+      el = $(el)
+      bucket = @buckets[d]
+      bucketLength = bucket?.length
 
-    .html (d) =>
-      return unless @buckets[d]
-      "<div class='label'>#{@buckets[d].length}</div>"
+      title = if bucketLength != 1
+        "Show #{bucketLength} annotations"
+      else if bucketLength > 0
+        'Show one annotation'
 
-    .addClass (d) => if @isUpper(d) then 'upper' else ''
-    .addClass (d) => if @isLower(d) then 'lower' else ''
+      el.attr('title', title)
+      el.toggleClass('upper', @isUpper(d))
+      el.toggleClass('lower', @isLower(d))
 
-    .css 'display', (d) =>
-      bucket = @buckets[d]
-      if (!bucket or bucket.length is 0) then 'none' else ''
+      el.css({
+        top: (@index[d] + @index[d+1]) / 2
+        marginTop: if @isUpper(d) or @isLower(d) then -9 else -8
+        display: unless bucketLength then 'none' else ''
+      })
 
-    if @dynamicBucket
-      @annotator.updateViewer this._getDynamicBucket()
+      if bucket
+        el.html("<div class='label'>#{bucketLength}</div>")
 
   _getDynamicBucket: ->
     top = window.pageYOffset

karma.config.js

@@ -47,6 +47,7 @@ module.exports = function(config) {
       'h/static/scripts/helpers.js',
       'h/static/scripts/session.js',
       'h/static/scripts/hypothesis.js',
+      'h/static/scripts/plugin/heatmap.js',
       'h/static/scripts/vendor/sinon.js',
       'h/static/scripts/vendor/chai.js',
       'h/templates/*.html',

tests/js/plugin/heatmap-test.coffee

+assert = chai.assert
+sinon.assert.expose(assert, prefix: '')
+
+describe 'Annotator.Heatmap', ->
+  createHeatmap = (options) ->
+    element = document.createElement('div')
+    new Annotator.Plugin.Heatmap(element, options || {})
+
+  # Yes this is testing a private method. Yes this is bad practice, but I'd
+  # rather test this functionality in a private method than not test it at all.
+  describe '_buildTabs', ->
+    setup = (tabs) ->
+      heatmap = createHeatmap()
+      heatmap.tabs = tabs
+      heatmap.buckets = [['AN ANNOTATION?']]
+      heatmap.index = [
+        0,
+        heatmap.BUCKET_THRESHOLD_PAD - 12,
+        heatmap.BUCKET_THRESHOLD_PAD + heatmap.BUCKET_SIZE + 6
+      ]
+      heatmap
+
+    it 'creates a tab with a title', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+
+      heatmap._buildTabs()
+      assert.equal(tab.attr('title'), 'Show one annotation')
+
+    it 'creates a tab with a pluralized title', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      heatmap.buckets[0].push('Another Annotation?')
+
+      heatmap._buildTabs()
+      assert.equal(tab.attr('title'), 'Show 2 annotations')
+
+    it 'sets the tab text to the number of annotations', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      heatmap.buckets[0].push('Another Annotation?')
+
+      heatmap._buildTabs()
+      assert.equal(tab.text(), '2')
+
+    it 'sets the tab text to the number of annotations', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      heatmap.buckets[0].push('Another Annotation?')
+
+      heatmap._buildTabs()
+      assert.equal(tab.text(), '2')
+
+    it 'adds the class "upper" if the annotation is at the top', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      sinon.stub(heatmap, 'isUpper').returns(true)
+
+      heatmap._buildTabs()
+      assert.equal(tab.hasClass('upper'), true)
+
+    it 'removes the class "upper" if the annotation is not at the top', ->
+      tab = $('<div />').addClass('upper')
+      heatmap = setup(tab)
+      sinon.stub(heatmap, 'isUpper').returns(false)
+
+      heatmap._buildTabs()
+      assert.equal(tab.hasClass('upper'), false)
+
+    it 'adds the class "lower" if the annotation is at the top', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      sinon.stub(heatmap, 'isLower').returns(true)
+
+      heatmap._buildTabs()
+      assert.equal(tab.hasClass('lower'), true)
+
+    it 'removes the class "lower" if the annotation is not at the top', ->
+      tab = $('<div />').addClass('lower')
+      heatmap = setup(tab)
+      sinon.stub(heatmap, 'isLower').returns(false)
+
+      heatmap._buildTabs()
+      assert.equal(tab.hasClass('lower'), false)
+
+    it 'reveals the tab if there are annotations in the bucket', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+
+      heatmap._buildTabs()
+      assert.equal(tab.css('display'), '')
+
+    it 'hides the tab if there are no annotations in the bucket', ->
+      tab = $('<div />')
+      heatmap = setup(tab)
+      heatmap.buckets = []
+
+      heatmap._buildTabs()
+      assert.equal(tab.css('display'), 'none')