Skip to content

Commit

Permalink
Separate anchored and unanchored annotations into tabs.
Browse files Browse the repository at this point in the history
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:<id>
   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:<orphan_id>
   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.
  • Loading branch information
Sheetal Umesh Kumar authored and robertknight committed Aug 4, 2016
1 parent 2e56bd4 commit a992507
Show file tree
Hide file tree
Showing 19 changed files with 252 additions and 53 deletions.
8 changes: 0 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
@@ -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&amp;hostname=irc.freenode.net&amp;port=6667&amp;ssl=1
Expand Down
31 changes: 28 additions & 3 deletions h/static/scripts/annotation-metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -137,7 +160,9 @@ module.exports = {
domainAndTitle: domainAndTitle,
isAnnotation: isAnnotation,
isNew: isNew,
isOrphan: isOrphan,
isPageNote: isPageNote,
isReply: isReply,
isWaitingToAnchor: isWaitingToAnchor,
location: location,
};
3 changes: 2 additions & 1 deletion h/static/scripts/annotation-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -62,7 +63,7 @@ function initialState(settings) {

filterQuery: null,

selectedTab: null,
selectedTab: uiConstants.TAB_ANNOTATIONS,

// Key by which annotations are currently sorted.
sortKey: 'Location',
Expand Down
7 changes: 5 additions & 2 deletions h/static/scripts/app-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
6 changes: 5 additions & 1 deletion h/static/scripts/directive/annotation.js
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,11 @@ function AnnotationController(
vm.target = function() {
return vm.annotation.target;
};


vm.isOrphan = function() {
return vm.annotation.$orphan;
};

vm.updated = function() {
return vm.annotation.updated;
};
Expand Down
1 change: 1 addition & 0 deletions h/static/scripts/directive/search-status-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
9 changes: 8 additions & 1 deletion h/static/scripts/directive/selection-tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
};
Expand Down
5 changes: 5 additions & 0 deletions h/static/scripts/directive/test/selection-tabs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});

Expand Down
32 changes: 23 additions & 9 deletions h/static/scripts/root-thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 43 additions & 0 deletions h/static/scripts/test/annotation-metadata-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

var annotationMetadata = require('../annotation-metadata');
var fixtures = require('./annotation-fixtures');

var documentMetadata = annotationMetadata.documentMetadata;
var domainAndTitle = annotationMetadata.domainAndTitle;
Expand Down Expand Up @@ -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()));
});
});
});
46 changes: 36 additions & 10 deletions h/static/scripts/test/root-thread-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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}}));
});
});

Expand Down
1 change: 1 addition & 0 deletions h/static/scripts/ui-constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@
module.exports = {
TAB_ANNOTATIONS: 'annotation',
TAB_NOTES: 'note',
TAB_ORPHANS: 'orphan',
};
Loading

0 comments on commit a992507

Please sign in to comment.