Skip to content

Commit

Permalink
Fix: Clarify separation between plain and comment highlights (#58)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pramodsum
pramodsum authored Dec 4, 2017
1 parent 93681b5 commit 496ceec
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 46 deletions.
16 changes: 8 additions & 8 deletions src/Annotator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ class Annotator extends EventEmitter {
options
});

this.handleControllerEvents = this.handleControllerEvents.bind(this);
controller.addListener('annotationcontrollerevent', this.handleControllerEvents);
});
}
Expand Down Expand Up @@ -360,12 +359,17 @@ class Annotator extends EventEmitter {
// Generate map of page to threads
Object.keys(threadMap).forEach((threadID) => {
const annotations = threadMap[threadID];
const firstAnnotation = util.getFirstAnnotation(annotations);
if (!firstAnnotation || !this.isModeAnnotatable(firstAnnotation.type)) {

// NOTE: Using the last annotation to evaluate if the annotation type
// is enabled because highlight comment annotations may have a plain
// highlight as the first annotation in the thread.
const lastAnnotation = util.getLastAnnotation(annotations);
if (!lastAnnotation || !this.isModeAnnotatable(lastAnnotation.type)) {
return;
}

// Bind events on valid annotation thread
const firstAnnotation = util.getLastAnnotation(annotations);
const thread = this.createAnnotationThread(annotations, firstAnnotation.location, firstAnnotation.type);
const controller = this.modeControllers[firstAnnotation.type];
if (controller) {
Expand Down Expand Up @@ -540,13 +544,9 @@ class Annotator extends EventEmitter {

const pageThreads = this.threads[pageNum] || {};
Object.keys(pageThreads).forEach((threadID) => {
const thread = pageThreads[threadID];
if (!this.isModeAnnotatable(thread.type)) {
return;
}

// Sets the annotatedElement if the thread was fetched before the
// dependent document/viewer finished loading
const thread = pageThreads[threadID];
if (!thread.annotatedElement) {
thread.annotatedElement = this.annotatedElement;
}
Expand Down
33 changes: 13 additions & 20 deletions src/__tests__/Annotator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,21 +307,6 @@ describe('Annotator', () => {
annotator.renderAnnotationsOnPage(1);
});

it('should not call show() if the thread type is disabled', () => {
const badType = 'not_accepted';
stubs.thread3.type = badType;
stubs.thread2.type = 'something';

stubs.threadMock3.expects('show').never();
stubs.threadMock2.expects('show').once();

const isModeAnn = sandbox.stub(annotator, 'isModeAnnotatable');
isModeAnn.withArgs(badType).returns(false);
isModeAnn.withArgs('something').returns(true);

annotator.renderAnnotationsOnPage('2');
});

it('should set annotatedElement if thread was fetched before it was set', () => {
stubs.thread2.annotatedElement = undefined;
stubs.threadMock2.expects('show').once();
Expand Down Expand Up @@ -473,9 +458,10 @@ describe('Annotator', () => {
describe('generateThreadMap()', () => {
beforeEach(() => {
stubs.threadMap = { '123abc': stubs.thread };
const annotation = { location: {}, type: 'something' };
const annotation = { location: {}, type: 'highlight' };
const lastAnnotation = { location: {}, type: 'highlight-comment' };
sandbox.stub(util, 'getFirstAnnotation').returns(annotation);
sandbox.stub(annotator, 'isModeAnnotatable').returns(true);
sandbox.stub(util, 'getLastAnnotation').returns(lastAnnotation);
});

it('should do nothing if annotator conf does not exist in options', () => {
Expand All @@ -486,19 +472,26 @@ describe('Annotator', () => {
});

it('should reset and create a new thread map by from annotations fetched from server', () => {
annotator.options.annotator = { NAME: 'name' };
annotator.options.annotator = { NAME: 'name', TYPE: ['highlight-comment'] };
sandbox.stub(annotator, 'createAnnotationThread').returns(stubs.thread);
annotator.generateThreadMap(stubs.threadMap);
expect(annotator.createAnnotationThread).to.be.called;
});

it('should register thread if controller exists', () => {
annotator.options.annotator = { NAME: 'name' };
annotator.modeControllers['something'] = stubs.controller;
annotator.options.annotator = { NAME: 'name', TYPE: ['highlight-comment'] };
annotator.modeControllers['highlight-comment'] = stubs.controller;
sandbox.stub(annotator, 'createAnnotationThread').returns(stubs.thread);
stubs.controllerMock.expects('registerThread');
annotator.generateThreadMap(stubs.threadMap);
});

it('should not register a highlight comment thread with a plain highlight for the first annotation', () => {
annotator.options.annotator = { NAME: 'name', TYPE: ['highlight'] };
annotator.modeControllers['highlight-comment'] = stubs.controller;
stubs.controllerMock.expects('registerThread').never();
annotator.generateThreadMap(stubs.threadMap);
});
});

describe('bindCustomListenersOnService()', () => {
Expand Down
11 changes: 11 additions & 0 deletions src/__tests__/util-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
getAvatarHtml,
getScale,
getFirstAnnotation,
getLastAnnotation,
isPlainHighlight,
isHighlightAnnotation,
getDimensionScale,
Expand Down Expand Up @@ -298,6 +299,16 @@ describe('util', () => {
});
});

describe('getLastAnnotation()', () => {
it('should return the last annotation in thread', () => {
const annotations = {
def123: { id: 1 },
abc456: { id: 2 }
};
expect(getLastAnnotation(annotations)).to.deep.equal({ id: 2 });
});
});

describe('isPlainHighlight()', () => {
it('should return true if highlight annotation is a plain highlight', () => {
const annotations = [{ text: '' }];
Expand Down
26 changes: 13 additions & 13 deletions src/doc/CreateHighlightDialog.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog {
constructor(parentEl, config = {}) {
super(parentEl, config);

this.allowHighlight = !!config.allowHighlight || true;
this.allowComment = !!config.allowComment || true;
this.allowHighlight = config.allowHighlight || false;
this.allowComment = config.allowComment || false;

// Explicit scope binding for event listeners
if (this.allowHighlight) {
Expand All @@ -92,6 +92,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog {
this.onCommentPost = this.onCommentPost.bind(this);
this.onCommentCancel = this.onCommentCancel.bind(this);
}

this.createElement();
}

/**
Expand Down Expand Up @@ -222,16 +224,6 @@ class CreateHighlightDialog extends CreateAnnotationDialog {
buttonsEl.appendChild(highlightEl);
}

if (this.allowComment) {
const commentEl = generateBtn(
[CLASS_BUTTON_PLAIN, CLASS_ADD_HIGHLIGHT_COMMENT_BTN],
this.localized.highlightComment,
ICON_HIGHLIGHT_COMMENT,
DATA_TYPE_ADD_HIGHLIGHT_COMMENT
);
buttonsEl.appendChild(commentEl);
}

const dialogEl = document.createElement('div');
dialogEl.classList.add(CLASS_ANNOTATION_HIGHLIGHT_DIALOG);
dialogEl.appendChild(buttonsEl);
Expand Down Expand Up @@ -264,8 +256,16 @@ class CreateHighlightDialog extends CreateAnnotationDialog {
}
}

// Events for comment button
if (this.allowComment) {
const commentEl = generateBtn(
[CLASS_BUTTON_PLAIN, CLASS_ADD_HIGHLIGHT_COMMENT_BTN],
this.localized.highlightComment,
ICON_HIGHLIGHT_COMMENT,
DATA_TYPE_ADD_HIGHLIGHT_COMMENT
);
buttonsEl.appendChild(commentEl);

// Events for comment button
this.commentBox = this.setupCommentBox(highlightDialogEl);

this.commentCreateEl = highlightDialogEl.querySelector(SELECTOR_ADD_HIGHLIGHT_COMMENT_BTN);
Expand Down
1 change: 0 additions & 1 deletion src/doc/DocAnnotator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,6 @@ class DocAnnotator extends Annotator {
allowHighlight: this.plainHighlightEnabled,
localized: this.localized
});
this.createHighlightDialog.createElement();

this.createHighlightDialog.addListener(CREATE_EVENT.init, () =>
this.emit(THREAD_EVENT.pending, TYPES.highlight)
Expand Down
13 changes: 9 additions & 4 deletions src/doc/__tests__/CreateHighlightDialog-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ describe('doc/CreateHighlightDialog', () => {

const parentEl = document.createElement('div');
parentEl.classList.add('bp-create-dialog-container');
dialog = new CreateHighlightDialog(parentEl, { localized });
dialog = new CreateHighlightDialog(parentEl, {
allowHighlight: true,
allowComment: true,
localized
});
});

afterEach(() => {
Expand All @@ -40,10 +44,10 @@ describe('doc/CreateHighlightDialog', () => {
});

describe('contructor()', () => {
it('should default to enable highlights and comments if no config passed in', () => {
it('should default to disable highlights and comments if no config passed in', () => {
const instance = new CreateHighlightDialog(document.createElement('div'));
expect(instance.allowHighlight).to.be.true;
expect(instance.allowComment).to.be.true;
expect(instance.allowHighlight).to.be.false;
expect(instance.allowComment).to.be.false;
});

it('should take config falsey value to disable highlights and comments, when passed in', () => {
Expand Down Expand Up @@ -228,6 +232,7 @@ describe('doc/CreateHighlightDialog', () => {

it('should not create a comment box or button if comments are disabled', () => {
dialog.allowComment = false;
dialog.commentBox = undefined;
dialog.createElement();

expect(dialog.containerEl.querySelector(`.${CLASS_ADD_HIGHLIGHT_COMMENT_BTN}`)).to.not.exist;
Expand Down
12 changes: 12 additions & 0 deletions src/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,18 @@ export function getFirstAnnotation(annotations) {
return annotations[firstAnnotationId];
}

/**
* Return first annotation in thread
*
* @param {Object} annotations - Annotations in thread
* @return {Annotation} First annotation in thread
*/
export function getLastAnnotation(annotations) {
const numAnnotations = Object.keys(annotations).length;
const lastAnnotationID = Object.keys(annotations)[numAnnotations - 1];
return annotations[lastAnnotationID];
}

/**
* Whether or not a highlight annotation has comments or is a plain highlight
*
Expand Down

0 comments on commit 496ceec

Please sign in to comment.