From 0a620b0735bd21fa4ce0b4fa09ee41e51dda9cf3 Mon Sep 17 00:00:00 2001 From: Sheetal Umesh Kumar Date: Thu, 21 Jul 2016 23:09:03 -0400 Subject: [PATCH] Separate anchored and unanchored annotations into tabs. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default to the annotations tab when user switches between groups, clears a selection or reloads a page. The ANNOTATIONS_SYNCED event is emitted when annotations for a document have finished anchoring. Because annotations are not marked as orphans until they fail to anchor, select the annotations tab after they have finished anchoring. Test cases: ------------- 1. On page load localhost:3000 annotations tab is selected. clicking on new note switches to the notes tab with a new draft created. switching between tabs maintains tab selection, if notes or annotations. when tab selected is orphans, changing groups defaults to the annotations tab. 2. On page load localhost:3000/#annotations: clicking on new page note should select the notes tab. When an annotation/note is selected, the tab selection carries between group changes. all tests listed in #1 above should apply. 3. On page load localhost:3000/#annotations: clicking on “show all annotations and notes” selects the annotations tab and displays all annotations. all tests listed in #1 and #2 above should apply. switching between groups selects the annotations tab by default https://trello.com/c/1NVK2jwv/400-add-a-tab-for-unanchored-annotations https://trello.com/c/OLdLTlLT/342-separate-annotations-and-notes Hide orphans tab behind orphans_tab flag. Fix bug where notes tab isn't selected when user attempts to create a new page note. --- README.md | 8 --- h/static/scripts/annotation-metadata.js | 31 ++++++++- h/static/scripts/annotation-ui.js | 3 +- h/static/scripts/app-controller.js | 7 +- h/static/scripts/directive/annotation.js | 6 +- .../scripts/directive/search-status-bar.js | 1 + h/static/scripts/directive/selection-tabs.js | 9 ++- .../directive/test/selection-tabs-test.js | 5 ++ h/static/scripts/root-thread.js | 32 ++++++--- .../scripts/test/annotation-metadata-test.js | 43 ++++++++++++ h/static/scripts/test/root-thread-test.js | 46 ++++++++++--- h/static/scripts/ui-constants.js | 1 + h/static/scripts/widget-controller.js | 68 ++++++++++++++++--- h/static/styles/annotation.scss | 4 ++ h/static/styles/selection-tabs.scss | 4 ++ h/templates/client/annotation.html | 1 + h/templates/client/search_status_bar.html | 2 +- h/templates/client/selection_tabs.html | 25 +++++-- h/templates/client/viewer.html | 9 ++- 19 files changed, 252 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index e7700dc32c1..e58ffb830ab 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,3 @@ -Hypothesis client -================= - -[![Build status](https://img.shields.io/travis/hypothesis/client/master.svg)][travis] -[![npm version](https://img.shields.io/npm/v/hypothesis.svg)][npm] -[![#hypothes.is IRC channel](https://img.shields.io/badge/IRC-%23hypothes.is-blue.svg)][irc] -[![BSD licensed](https://img.shields.io/badge/license-BSD-blue.svg)][license] - [travis]: https://travis-ci.org/hypothesis/client [npm]: https://www.npmjs.com/package/hypothesis [irc]: https://www.irccloud.com/invite?channel=%23hypothes.is&hostname=irc.freenode.net&port=6667&ssl=1 diff --git a/h/static/scripts/annotation-metadata.js b/h/static/scripts/annotation-metadata.js index 90b4468dee8..54b5cf13eb9 100644 --- a/h/static/scripts/annotation-metadata.js +++ b/h/static/scripts/annotation-metadata.js @@ -101,14 +101,37 @@ function isNew(annotation) { return !annotation.id; } -/** Return `true` if the given annotation is a page note, `false` otherwise. */ +/** + * Return `true` if `annotation` has a selector. + * + * An annotation which has a selector refers to a specific part of a document, + * as opposed to a Page Note which refers to the whole document or a reply, + * which refers to another annotation. + */ +function hasSelector(annotation) { + return !!(annotation.target && + annotation.target.length > 0 && + annotation.target[0].selector); +} + +/** Return `true` if the given annotation is not yet anchored. */ +function isWaitingToAnchor(annotation) { + return hasSelector(annotation) && (typeof annotation.$orphan === 'undefined'); +} + +/** Return `true` if the given annotation is an orphan. */ +function isOrphan(annotation) { + return hasSelector(annotation) && annotation.$orphan; +} + +/** Return `true` if the given annotation is a page note. */ function isPageNote(annotation) { - return !isAnnotation(annotation) && !isReply(annotation); + return !hasSelector(annotation) && !isReply(annotation); } /** Return `true` if the given annotation is a top level annotation, `false` otherwise. */ function isAnnotation(annotation) { - return !!(annotation.target && annotation.target.length > 0 && annotation.target[0].selector); + return !!(hasSelector(annotation) && !isOrphan(annotation)); } /** Return a numeric key that can be used to sort annotations by location. @@ -137,7 +160,9 @@ module.exports = { domainAndTitle: domainAndTitle, isAnnotation: isAnnotation, isNew: isNew, + isOrphan: isOrphan, isPageNote: isPageNote, isReply: isReply, + isWaitingToAnchor: isWaitingToAnchor, location: location, }; diff --git a/h/static/scripts/annotation-ui.js b/h/static/scripts/annotation-ui.js index 003b72ed3db..75b395b15a1 100644 --- a/h/static/scripts/annotation-ui.js +++ b/h/static/scripts/annotation-ui.js @@ -10,6 +10,7 @@ var immutable = require('seamless-immutable'); var redux = require('redux'); +var uiConstants = require('./ui-constants'); function freeze(selection) { if (Object.keys(selection).length) { @@ -62,7 +63,7 @@ function initialState(settings) { filterQuery: null, - selectedTab: null, + selectedTab: uiConstants.TAB_ANNOTATIONS, // Key by which annotations are currently sorted. sortKey: 'Location', diff --git a/h/static/scripts/app-controller.js b/h/static/scripts/app-controller.js index c3a22de2e36..8f3244e9cd5 100644 --- a/h/static/scripts/app-controller.js +++ b/h/static/scripts/app-controller.js @@ -137,10 +137,13 @@ module.exports = function AppController( }; $scope.clearSelection = function () { - if (!annotationUI.getState().selectedTab) { - annotationUI.selectTab(uiConstants.TAB_ANNOTATIONS); + var selectedTab = annotationUI.getState().selectedTab; + if (!annotationUI.getState().selectedTab || annotationUI.getState().selectedTab === uiConstants.TAB_ORPHANS) { + selectedTab = uiConstants.TAB_ANNOTATIONS; } + annotationUI.clearSelectedAnnotations(); + annotationUI.selectTab(selectedTab); }; $scope.search = { diff --git a/h/static/scripts/directive/annotation.js b/h/static/scripts/directive/annotation.js index b7166a7814c..d9bc5f47d38 100644 --- a/h/static/scripts/directive/annotation.js +++ b/h/static/scripts/directive/annotation.js @@ -443,7 +443,11 @@ function AnnotationController( vm.target = function() { return vm.annotation.target; }; - + + vm.isOrphan = function() { + return vm.annotation.$orphan; + }; + vm.updated = function() { return vm.annotation.updated; }; diff --git a/h/static/scripts/directive/search-status-bar.js b/h/static/scripts/directive/search-status-bar.js index b53ba2a1d82..e581702bce0 100644 --- a/h/static/scripts/directive/search-status-bar.js +++ b/h/static/scripts/directive/search-status-bar.js @@ -10,6 +10,7 @@ module.exports = function () { controller: function () { this.TAB_ANNOTATIONS = uiConstants.TAB_ANNOTATIONS; this.TAB_NOTES = uiConstants.TAB_NOTES; + this.TAB_ORPHANS = uiConstants.TAB_ORPHANS; }, restrict: 'E', scope: { diff --git a/h/static/scripts/directive/selection-tabs.js b/h/static/scripts/directive/selection-tabs.js index e6721ce4776..188647c32c2 100644 --- a/h/static/scripts/directive/selection-tabs.js +++ b/h/static/scripts/directive/selection-tabs.js @@ -7,21 +7,28 @@ module.exports = function () { bindToController: true, controllerAs: 'vm', //@ngInject - controller: function ($element, annotationUI) { + controller: function ($element, annotationUI, features) { this.TAB_ANNOTATIONS = uiConstants.TAB_ANNOTATIONS; this.TAB_NOTES = uiConstants.TAB_NOTES; + this.TAB_ORPHANS = uiConstants.TAB_ORPHANS; this.selectTab = function (type) { annotationUI.clearSelectedAnnotations(); annotationUI.selectTab(type); }; + + this.orphansTabFlagEnabled = function () { + return features.flagEnabled('orphans_tab'); + }; }, restrict: 'E', scope: { isLoading: '<', + isWaitingToAnchorAnnotations: '<', selectedTab: '<', totalAnnotations: '<', totalNotes: '<', + totalOrphans: '<', }, template: require('../../../templates/client/selection_tabs.html'), }; diff --git a/h/static/scripts/directive/test/selection-tabs-test.js b/h/static/scripts/directive/test/selection-tabs-test.js index 791cc062e4c..fcb64e44773 100644 --- a/h/static/scripts/directive/test/selection-tabs-test.js +++ b/h/static/scripts/directive/test/selection-tabs-test.js @@ -12,8 +12,13 @@ describe('selectionTabs', function () { beforeEach(function () { var fakeAnnotationUI = {}; + var fakeFeatures = { + flagEnabled: sinon.stub().returns(true), + }; + angular.mock.module('app', { annotationUI: fakeAnnotationUI, + features: fakeFeatures, }); }); diff --git a/h/static/scripts/root-thread.js b/h/static/scripts/root-thread.js index af7b5e3746a..6526abc43bd 100644 --- a/h/static/scripts/root-thread.js +++ b/h/static/scripts/root-thread.js @@ -60,15 +60,29 @@ function RootThread($rootScope, annotationUI, features, searchFilter, viewFilter var threadFilterFn; if (features.flagEnabled('selection_tabs') && !state.filterQuery && state.selectedTab) { - threadFilterFn = function (thread) { - if (state.selectedTab === uiConstants.TAB_ANNOTATIONS) { - return thread.annotation && metadata.isAnnotation(thread.annotation); - } else if (state.selectedTab === uiConstants.TAB_NOTES) { - return thread.annotation && metadata.isPageNote(thread.annotation); - } else { - throw new Error('Invalid selected tab'); - } - }; + if (!features.flagEnabled('orphans_tab')) { + threadFilterFn = function (thread) { + if (state.selectedTab === uiConstants.TAB_ANNOTATIONS || state.selectedTab === uiConstants.TAB_ORPHANS) { + return thread.annotation && (metadata.isAnnotation(thread.annotation) || metadata.isOrphan(thread.annotation)); + } else if (state.selectedTab === uiConstants.TAB_NOTES) { + return thread.annotation && metadata.isPageNote(thread.annotation); + } else { + throw new Error('Invalid selected tab'); + } + }; + } else { + threadFilterFn = function (thread) { + if (state.selectedTab === uiConstants.TAB_ANNOTATIONS) { + return thread.annotation && metadata.isAnnotation(thread.annotation); + } else if (state.selectedTab === uiConstants.TAB_NOTES) { + return thread.annotation && metadata.isPageNote(thread.annotation); + } else if (state.selectedTab === uiConstants.TAB_ORPHANS) { + return thread.annotation && metadata.isOrphan(thread.annotation); + } else { + throw new Error('Invalid selected tab'); + } + }; + } } // Get the currently loaded annotations and the set of inputs which diff --git a/h/static/scripts/test/annotation-metadata-test.js b/h/static/scripts/test/annotation-metadata-test.js index d560076c59d..1b971cb59ae 100644 --- a/h/static/scripts/test/annotation-metadata-test.js +++ b/h/static/scripts/test/annotation-metadata-test.js @@ -1,6 +1,7 @@ 'use strict'; var annotationMetadata = require('../annotation-metadata'); +var fixtures = require('./annotation-fixtures'); var documentMetadata = annotationMetadata.documentMetadata; var domainAndTitle = annotationMetadata.domainAndTitle; @@ -245,4 +246,46 @@ describe('annotation-metadata', function () { assert.isFalse(annotationMetadata.isAnnotation({})); }); }); + + describe('.isOrphan', function () { + it('returns true if an annotation was anchored', function () { + var annotation = Object.assign(fixtures.defaultAnnotation(), {$orphan: true}); + assert.isTrue(annotationMetadata.isOrphan(annotation)); + }); + + it('returns false if an annotation failed to anchor', function() { + var orphan = Object.assign(fixtures.defaultAnnotation(), {$orphan: false}); + assert.isFalse(annotationMetadata.isOrphan(orphan)); + }); + }); + + describe('.isWaitingToAnchor', function () { + var isWaitingToAnchor = annotationMetadata.isWaitingToAnchor; + + it('returns true for annotations that are not yet anchored', function () { + assert.isTrue(isWaitingToAnchor(fixtures.defaultAnnotation())); + }); + + it('returns false for annotations that are anchored', function () { + var anchored = Object.assign({}, fixtures.defaultAnnotation(), { + $orphan: false, + }); + assert.isFalse(isWaitingToAnchor(anchored)); + }); + + it('returns false for annotations that failed to anchor', function () { + var anchored = Object.assign({}, fixtures.defaultAnnotation(), { + $orphan: true, + }); + assert.isFalse(isWaitingToAnchor(anchored)); + }); + + it('returns false for replies', function () { + assert.isFalse(isWaitingToAnchor(fixtures.oldReply())); + }); + + it('returns false for page notes', function () { + assert.isFalse(isWaitingToAnchor(fixtures.oldPageNote())); + }); + }); }); diff --git a/h/static/scripts/test/root-thread-test.js b/h/static/scripts/test/root-thread-test.js index 7d874ac1ed7..999fddfab51 100644 --- a/h/static/scripts/test/root-thread-test.js +++ b/h/static/scripts/test/root-thread-test.js @@ -6,6 +6,7 @@ var immutable = require('seamless-immutable'); var annotationFixtures = require('./annotation-fixtures'); var events = require('../events'); +var uiConstants = require('../ui-constants'); var util = require('./util'); var unroll = util.unroll; @@ -186,11 +187,11 @@ describe('rootThread', function () { }); describe('when the thread filter query is set', function () { - it('generates a thread filter function to match annotations', function () { + it('filter matches only annotations when Annotations tab is selected', function () { fakeBuildThread.reset(); fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, - {selectedTab: 'annotation'}); + {selectedTab: uiConstants.TAB_ANNOTATIONS}); rootThread.thread(fakeAnnotationUI.state); var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn; @@ -199,30 +200,55 @@ describe('rootThread', function () { assert.isDefined(threadFilterFn({annotation: annotation})); }); - it('generates a thread filter function to match notes', function () { + it('filter matches only notes when Notes tab is selected', function () { fakeBuildThread.reset(); - fakeBuildThread.annotation = {target: [{}]}; fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, - {selectedTab: 'note'}); + {selectedTab: uiConstants.TAB_NOTES}); rootThread.thread(fakeAnnotationUI.state); var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn; - assert.isTrue(threadFilterFn(fakeBuildThread)); + assert.isTrue(threadFilterFn({annotation: {target: [{}]}})); }); - it('generates a thread filter function for annotations, when all annotations are of type notes', function () { + it('filter matches only orphans when Orphans tab is selected', function () { fakeBuildThread.reset(); - fakeBuildThread.annotation = {target: [{}]}; fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, - {selectedTab: 'annotation'}); + {selectedTab: uiConstants.TAB_ORPHANS}); rootThread.thread(fakeAnnotationUI.state); var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn; - assert.isFalse(threadFilterFn(fakeBuildThread)); + var orphan = Object.assign(annotationFixtures.defaultAnnotation(), + {$orphan: true}); + + assert.isTrue(threadFilterFn({annotation: orphan})); + }); + + it('filter matches only notes when Annotations tab is selected', function () { + fakeBuildThread.reset(); + + fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, + {selectedTab: uiConstants.TAB_ANNOTATIONS}); + + rootThread.thread(fakeAnnotationUI.state); + var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn; + + assert.isFalse(threadFilterFn({annotation: {target: [{}]}})); + }); + + it('filter matches only orphans when Annotations tab is selected', function () { + fakeBuildThread.reset(); + + fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state, + {selectedTab: uiConstants.TAB_ANNOTATIONS}); + + rootThread.thread(fakeAnnotationUI.state); + var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn; + + assert.isFalse(threadFilterFn({annotation: {$orphan: true}})); }); }); diff --git a/h/static/scripts/ui-constants.js b/h/static/scripts/ui-constants.js index 6e68f4e43dc..4fbd1e0c0e7 100644 --- a/h/static/scripts/ui-constants.js +++ b/h/static/scripts/ui-constants.js @@ -7,4 +7,5 @@ module.exports = { TAB_ANNOTATIONS: 'annotation', TAB_NOTES: 'note', + TAB_ORPHANS: 'orphan', }; diff --git a/h/static/scripts/widget-controller.js b/h/static/scripts/widget-controller.js index d065beebcee..170be5bbd7b 100644 --- a/h/static/scripts/widget-controller.js +++ b/h/static/scripts/widget-controller.js @@ -34,11 +34,31 @@ function groupIDFromSelection(selection, results) { // @ngInject module.exports = function WidgetController( - $scope, $rootScope, annotationUI, crossframe, annotationMapper, - drafts, features, groups, rootThread, settings, streamer, streamFilter, store, + $scope, annotationUI, crossframe, annotationMapper, drafts, + features, groups, rootThread, settings, streamer, streamFilter, store, VirtualThreadList ) { + /** + * Returns the number of annotations which are not yet anchored. + */ + function countWaitingToAnchorAnnotations (annotations) { + var total = annotations.reduce(function (count, annotation) { + return annotation && metadata.isWaitingToAnchor(annotation) ? count + 1 : count; + }, 0); + return total; + } + + /** + * Returns the number of top level annotations which are unanchored i.e orphans. + */ + function countOrphans(annotations) { + var total = annotations.reduce(function (count, annotation) { + return annotation && metadata.isOrphan(annotation) ? count + 1 : count; + }, 0); + return total; + } + /** * Returns the number of top level annotations which are of type annotations * and not notes or replies. @@ -97,8 +117,16 @@ module.exports = function WidgetController( visibleThreads.setRootThread(thread()); $scope.selectedTab = annotationUI.getState().selectedTab; - $scope.totalAnnotations = countAnnotations(annotationUI.getState().annotations); + $scope.waitingToAnchorAnnotations = countWaitingToAnchorAnnotations(annotationUI.getState().annotations) > 0; + $scope.totalNotes = countNotes(annotationUI.getState().annotations); + $scope.totalOrphans = countOrphans(annotationUI.getState().annotations); + + if (!features.flagEnabled('orphans_tab')) { + $scope.totalAnnotations = $scope.totalAnnotations + $scope.totalOrphans; + } else { + $scope.totalAnnotations = countAnnotations(annotationUI.getState().annotations); + } }); $scope.$on('$destroy', unsubscribeAnnotationUI); @@ -158,7 +186,9 @@ module.exports = function WidgetController( return null; } - if (metadata.isAnnotation(annot)) { + if (metadata.isOrphan(annot)) { + return uiConstants.TAB_ORPHANS; + } else if (metadata.isAnnotation(annot)) { return uiConstants.TAB_ANNOTATIONS; } else if (metadata.isPageNote(annot)) { return uiConstants.TAB_NOTES; @@ -167,6 +197,14 @@ module.exports = function WidgetController( } } + function selectAppropriateTab() { + if (annotationUI.getState().selectedTab === uiConstants.TAB_ORPHANS) { + annotationUI.selectTab(uiConstants.TAB_ANNOTATIONS); + } else { + annotationUI.selectTab(annotationUI.getState().selectedTab); + } + } + /** * Returns the Annotation object for the first annotation in the * selected annotation set. Note that 'first' refers to the order @@ -204,9 +242,15 @@ module.exports = function WidgetController( searchClients.push(searchClient); searchClient.on('results', function (results) { if (annotationUI.hasSelectedAnnotations()) { - // Select appropriate tab - notes or annotations, for selection - annotationUI.selectTab( - tabTypeFromSelection(annotationUI.getState().selectedAnnotationMap, results)); + // Select appropriate tab - notes, annotations or orphans, for selection + $scope.$on(events.ANNOTATIONS_SYNCED, function () { + var tabToSelect = tabTypeFromSelection(annotationUI.getState().selectedAnnotationMap, results); + if (tabToSelect) { + annotationUI.selectTab(tabToSelect); + } else { + selectAppropriateTab(); + } + }); // Focus the group containing the selected annotation and filter // annotations to those from this group @@ -314,9 +358,11 @@ module.exports = function WidgetController( streamer.reconnect(); }); - // When a direct-linked annotation is successfully anchored in the page, - // focus and scroll to it $scope.$on(events.ANNOTATIONS_SYNCED, function (event, tags) { + selectAppropriateTab(); + + // When a direct-linked annotation is successfully anchored in the page, + // focus and scroll to it var selectedAnnot = firstSelectedAnnotation(); if (!selectedAnnot) { return; @@ -339,9 +385,10 @@ module.exports = function WidgetController( if (isLoading()) { return; } - annotationUI.clearSelectedAnnotations(); loadAnnotations(crossframe.frames); + + selectAppropriateTab(); }); // Watch anything that may require us to reload annotations. @@ -407,7 +454,6 @@ module.exports = function WidgetController( }; $scope.isLoading = isLoading; - annotationUI.selectTab(uiConstants.TAB_ANNOTATIONS); var visibleCount = memoize(function (thread) { return thread.children.reduce(function (count, child) { diff --git a/h/static/styles/annotation.scss b/h/static/styles/annotation.scss index 269d2b809b7..fa2453667ac 100644 --- a/h/static/styles/annotation.scss +++ b/h/static/styles/annotation.scss @@ -140,6 +140,10 @@ margin-bottom: $layout-h-margin - 3px; } +.annotation-quote-list.is-orphan { + text-decoration: line-through; +} + .annotation-media-embed { width: 369px; height: 208px; diff --git a/h/static/styles/selection-tabs.scss b/h/static/styles/selection-tabs.scss index 778af686121..281aea78c99 100644 --- a/h/static/styles/selection-tabs.scss +++ b/h/static/styles/selection-tabs.scss @@ -40,3 +40,7 @@ position: relative; top: 10px; } + +.selection-tabs__type--orphan { + margin-left: -5px; +} diff --git a/h/templates/client/annotation.html b/h/templates/client/annotation.html index 4f44ff0b28c..800d763affb 100644 --- a/h/templates/client/annotation.html +++ b/h/templates/client/annotation.html @@ -51,6 +51,7 @@
- + Show all annotations and notes diff --git a/h/templates/client/selection_tabs.html b/h/templates/client/selection_tabs.html index efa7d22896f..bd9fbaac6d6 100644 --- a/h/templates/client/selection_tabs.html +++ b/h/templates/client/selection_tabs.html @@ -3,16 +3,33 @@ + h-on-touch="vm.selectTab(vm.TAB_ANNOTATIONS)"> Annotations - {{ vm.totalAnnotations }} + + {{ vm.totalAnnotations }} + + h-on-touch="vm.selectTab(vm.TAB_NOTES)"> Page Notes - {{ vm.totalNotes }} + + {{ vm.totalNotes }} + + + + Orphans + + {{ vm.totalOrphans }} +
diff --git a/h/templates/client/viewer.html b/h/templates/client/viewer.html index 96472228f85..c91689c1bf4 100644 --- a/h/templates/client/viewer.html +++ b/h/templates/client/viewer.html @@ -1,8 +1,12 @@ - + total-notes="totalNotes" + total-orphans="totalOrphans">