From 66ab1ede24182a670434bbc3d5fab909ad46c2a6 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 28 Sep 2018 14:25:40 -0700 Subject: [PATCH 1/6] Update: Adding headerElement to annotator options --- src/Annotator.js | 7 +++++++ src/controllers/AnnotationModeController.js | 5 +++-- src/controllers/DrawingModeController.js | 6 +++--- src/controllers/PointModeController.js | 6 +++--- src/controllers/__tests__/AnnotationModeController-test.js | 6 +++--- src/controllers/__tests__/PointModeController-test.js | 6 ++++-- 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/Annotator.js b/src/Annotator.js index 464324412..d7bb8bab1 100644 --- a/src/Annotator.js +++ b/src/Annotator.js @@ -106,6 +106,12 @@ class Annotator extends EventEmitter { this.container = document.querySelector(this.options.container); } + // Get the header dom element if selector was passed, in tests + this.headerElement = this.options.headerElement; + if (typeof this.options.headerElement === 'string') { + this.headerElement = document.querySelector(this.options.headerElement); + } + // Get annotated element from container this.annotatedElement = this.getAnnotatedEl(this.container); @@ -243,6 +249,7 @@ class Annotator extends EventEmitter { const controller = this.modeControllers[type]; controller.init({ container: this.container, + headerElement: this.headerElement, annotatedElement: this.annotatedElement, mode: type, modeButton: this.modeButtons[type], diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 07eab09d5..9b2a7de2b 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -38,6 +38,7 @@ class AnnotationModeController extends EventEmitter { */ init(data) { this.container = data.container; + this.headerElement = data.headerElement; this.annotatedElement = data.annotatedElement; this.mode = data.mode; this.annotator = data.annotator; @@ -78,7 +79,7 @@ class AnnotationModeController extends EventEmitter { * @return {HTMLElement|null} Annotate button element or null if the selector did not find an element. */ getButton(annotatorSelector) { - return this.container.querySelector(annotatorSelector); + return this.headerElement.querySelector(annotatorSelector); } /** @@ -126,7 +127,7 @@ class AnnotationModeController extends EventEmitter { */ exit() { this.emit(CONTROLLER_EVENT.exit, { mode: this.mode }); - replaceHeader(this.container, SELECTOR_BOX_PREVIEW_BASE_HEADER); + replaceHeader(this.headerElement, SELECTOR_BOX_PREVIEW_BASE_HEADER); this.destroyPendingThreads(); diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index 2295c55e9..07a090acf 100644 --- a/src/controllers/DrawingModeController.js +++ b/src/controllers/DrawingModeController.js @@ -46,8 +46,8 @@ class DrawingModeController extends AnnotationModeController { // light, dark, or no value given), then we want to use our draw // header. Otherwise we expect header UI to be handled by Preview’s // consumer - if (data.options.header !== 'none') { - this.setupHeader(this.container, shell); + if (data.options.header !== 'none' || this.headerElement) { + this.setupHeader(this.headerElement, shell); } this.handleSelection = this.handleSelection.bind(this); @@ -250,7 +250,7 @@ class DrawingModeController extends AnnotationModeController { */ enter() { super.enter(); - replaceHeader(this.container, SELECTOR_DRAW_MODE_HEADER); + replaceHeader(this.headerElement, SELECTOR_DRAW_MODE_HEADER); this.annotatedElement.classList.add(CLASS_ANNOTATION_DRAW_MODE); } diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js index 18057cb76..0549dd5c4 100644 --- a/src/controllers/PointModeController.js +++ b/src/controllers/PointModeController.js @@ -25,8 +25,8 @@ class PointModeController extends AnnotationModeController { // light, dark, or no value given), then we want to use our draw // header. Otherwise we expect header UI to be handled by Preview’s // consumer - if (data.options.header !== 'none') { - this.setupHeader(this.container, shell); + if (data.options.header !== 'none' || this.headerElement) { + this.setupHeader(this.headerElement, shell); } } @@ -138,7 +138,7 @@ class PointModeController extends AnnotationModeController { /** @inheritdoc */ enter() { super.enter(); - replaceHeader(this.container, SELECTOR_POINT_MODE_HEADER); + replaceHeader(this.headerElement, SELECTOR_POINT_MODE_HEADER); this.annotatedElement.classList.add(CLASS_ANNOTATION_POINT_MODE); if (this.buttonEl) { diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index 7d1ae59b1..ac2b681e8 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/src/controllers/__tests__/AnnotationModeController-test.js @@ -95,8 +95,8 @@ describe('controllers/AnnotationModeController', () => { it('should return the annotation mode button', () => { const buttonEl = document.createElement('button'); buttonEl.classList.add('class'); - controller.container = document.createElement('div'); - controller.container.appendChild(buttonEl); + controller.headerElement = document.createElement('div'); + controller.headerElement.appendChild(buttonEl); expect(controller.getButton('.class')).not.toBeNull(); }); @@ -178,7 +178,7 @@ describe('controllers/AnnotationModeController', () => { expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.exit, expect.any(Object)); expect(controller.unbindListeners).toBeCalled(); expect(controller.emit).toBeCalledWith('binddomlisteners'); - expect(util.replaceHeader).toBeCalledWith(controller.container, SELECTOR_BOX_PREVIEW_BASE_HEADER); + expect(util.replaceHeader).toBeCalledWith(controller.headerElement, SELECTOR_BOX_PREVIEW_BASE_HEADER); }); }); diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js index 17156e036..f85c01808 100644 --- a/src/controllers/__tests__/PointModeController-test.js +++ b/src/controllers/__tests__/PointModeController-test.js @@ -144,6 +144,7 @@ describe('controllers/PointModeController', () => { // Set up annotation mode controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); + controller.headerElement = document.createElement('div'); controller.buttonEl = document.createElement('button'); controller.buttonEl.classList.add(CLASS_ACTIVE); @@ -180,6 +181,7 @@ describe('controllers/PointModeController', () => { controller.annotatedElement = document.createElement('div'); controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE); + controller.headerElement = document.createElement('div'); controller.buttonEl = document.createElement('button'); controller.buttonEl.classList.add(CLASS_ACTIVE); @@ -189,14 +191,14 @@ describe('controllers/PointModeController', () => { controller.enter(); expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.enter, expect.any(Object)); expect(controller.bindListeners).toBeCalled(); - expect(util.replaceHeader).toBeCalledWith(controller.container, SELECTOR_POINT_MODE_HEADER); + expect(util.replaceHeader).toBeCalledWith(controller.headerElement, SELECTOR_POINT_MODE_HEADER); }); it('should activate mode button if available', () => { controller.buttonEl = document.createElement('button'); controller.enter(); expect(controller.buttonEl.classList).toContain(CLASS_ACTIVE); - expect(util.replaceHeader).toBeCalledWith(controller.container, SELECTOR_POINT_MODE_HEADER); + expect(util.replaceHeader).toBeCalledWith(controller.headerElement, SELECTOR_POINT_MODE_HEADER); }); }); From 3f1c48264c023da29bdd319cce1a522b033d53c5 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 1 Oct 2018 14:41:20 -0700 Subject: [PATCH 2/6] Update: headerElement fallback to container when using preview header --- src/Annotator.js | 6 ++++++ src/__tests__/Annotator-test.js | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/Annotator.js b/src/Annotator.js index d7bb8bab1..3241258ab 100644 --- a/src/Annotator.js +++ b/src/Annotator.js @@ -112,6 +112,12 @@ class Annotator extends EventEmitter { this.headerElement = document.querySelector(this.options.headerElement); } + // If using box content preview header and no external header element was specified, + // fallback to the container element + if (this.options.header !== 'none' && !this.options.headerElement) { + this.headerElement = this.container; + } + // Get annotated element from container this.annotatedElement = this.getAnnotatedEl(this.container); diff --git a/src/__tests__/Annotator-test.js b/src/__tests__/Annotator-test.js index 2daa793e4..8b87e04b8 100644 --- a/src/__tests__/Annotator-test.js +++ b/src/__tests__/Annotator-test.js @@ -112,6 +112,12 @@ describe('Annotator', () => { expect(annotator.loadAnnotations).toBeCalled(); }); + it('should set the headerElement to the container as a fallback', () => { + annotator.options.header = 'light'; + annotator.init(5); + expect(annotator.headerElement).toEqual(document); + }); + it('should setup mobile dialog for mobile browsers', () => { annotator.isMobile = true; annotator.init(); From 5e75aa9508db18f88e9702a7021a7aea5c226522 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 4 Oct 2018 15:18:13 -0700 Subject: [PATCH 3/6] Chore: hide the annotation button on destroy --- src/controllers/AnnotationModeController.js | 18 +++++++ .../AnnotationModeController-test.js | 47 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 9b2a7de2b..8ce685caf 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -70,6 +70,8 @@ class AnnotationModeController extends EventEmitter { if (this.buttonEl) { this.buttonEl.removeEventListener('click', this.toggleMode); } + + this.hideButton(); } /** @@ -102,6 +104,22 @@ class AnnotationModeController extends EventEmitter { } } + /** + * Hides the annotate button for the specified mode + * + * @return {void} + */ + hideButton() { + if (!this.permissions.canAnnotate) { + return; + } + + this.buttonEl = this.getButton(this.modeButton.selector); + if (this.buttonEl) { + this.buttonEl.classList.add(CLASS_HIDDEN); + } + } + /** * Toggles annotation modes on and off. When an annotation mode is * on, annotation threads will be created at that location. diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index ac2b681e8..d40fb4e72 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/src/controllers/__tests__/AnnotationModeController-test.js @@ -73,6 +73,10 @@ describe('controllers/AnnotationModeController', () => { }); describe('destroy()', () => { + beforeEach(() => { + controller.hideButton = jest.fn(); + }); + it('should destroy all the threads in controller', () => { // eslint-disable-next-line new-cap controller.threads = { 1: new rbush() }; @@ -89,6 +93,11 @@ describe('controllers/AnnotationModeController', () => { controller.destroy(); expect(controller.buttonEl.removeEventListener).toBeCalled(); }); + + it('should hide the button', () => { + controller.destroy(); + expect(controller.hideButton).toBeCalled(); + }); }); describe('getButton', () => { @@ -141,6 +150,44 @@ describe('controllers/AnnotationModeController', () => { }); }); + describe('hideButton()', () => { + let buttonEl; + + beforeEach(() => { + controller.modeButton = { + type: { + title: 'Annotation Mode', + selector: '.selector' + } + }; + buttonEl = document.createElement('button'); + buttonEl.title = controller.modeButton.title; + // buttonEl.classList.add(CLASS_HIDDEN); + buttonEl.classList.add('selector'); + buttonEl.addEventListener = jest.fn(); + + controller.permissions = { canAnnotate: true }; + controller.getButton = jest.fn().mockReturnValue(buttonEl); + }); + + it('should do nothing if user cannot annotate', () => { + controller.permissions.canAnnotate = false; + controller.hideButton(); + expect(buttonEl.classList).not.toContain(CLASS_HIDDEN); + }); + + it('should do nothing if button is not found', () => { + controller.getButton = jest.fn(); + controller.hideButton(); + expect(buttonEl.classList).not.toContain(CLASS_HIDDEN); + }); + + it('should add the bp-is-hidden class to the button', () => { + controller.hideButton(); + expect(buttonEl.classList).toContain(CLASS_HIDDEN); + }); + }); + describe('toggleMode()', () => { beforeEach(() => { controller.emit = jest.fn(); From 36417b00f28ec801ad2fbe68e5c9bdbb23a2b724 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 5 Oct 2018 13:56:11 -0700 Subject: [PATCH 4/6] Chore: default to header container --- src/Annotator.js | 5 +++-- src/__tests__/Annotator-test.js | 8 +++++--- src/constants.js | 2 ++ 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Annotator.js b/src/Annotator.js index 3241258ab..97283f25d 100644 --- a/src/Annotator.js +++ b/src/Annotator.js @@ -14,7 +14,8 @@ import { THREAD_EVENT, ANNOTATOR_EVENT, CONTROLLER_EVENT, - CLASS_ANNOTATIONS_LOADED + CLASS_ANNOTATIONS_LOADED, + SELECTOR_BOX_PREVIEW_HEADER_CONTAINER } from './constants'; class Annotator extends EventEmitter { @@ -115,7 +116,7 @@ class Annotator extends EventEmitter { // If using box content preview header and no external header element was specified, // fallback to the container element if (this.options.header !== 'none' && !this.options.headerElement) { - this.headerElement = this.container; + this.headerElement = this.container.querySelector(SELECTOR_BOX_PREVIEW_HEADER_CONTAINER); } // Get annotated element from container diff --git a/src/__tests__/Annotator-test.js b/src/__tests__/Annotator-test.js index 8b87e04b8..8d8104a37 100644 --- a/src/__tests__/Annotator-test.js +++ b/src/__tests__/Annotator-test.js @@ -7,13 +7,15 @@ import { ANNOTATOR_EVENT, THREAD_EVENT, CONTROLLER_EVENT, - SELECTOR_ANNOTATED_ELEMENT + SELECTOR_ANNOTATED_ELEMENT, + SELECTOR_BOX_PREVIEW_HEADER_CONTAINER } from '../constants'; let annotator; let controller; let thread; -const html = ` +const html = `
+
`; describe('Annotator', () => { @@ -115,7 +117,7 @@ describe('Annotator', () => { it('should set the headerElement to the container as a fallback', () => { annotator.options.header = 'light'; annotator.init(5); - expect(annotator.headerElement).toEqual(document); + expect(annotator.headerElement).toEqual(document.querySelector(SELECTOR_BOX_PREVIEW_HEADER_CONTAINER)); }); it('should setup mobile dialog for mobile browsers', () => { diff --git a/src/constants.js b/src/constants.js index a61857eb9..8c1c8c312 100644 --- a/src/constants.js +++ b/src/constants.js @@ -27,6 +27,8 @@ export const CLASS_ANNOTATION_BUTTON_DRAW = 'bp-btn-annotate-draw'; export const SELECTOR_ANNOTATION_BUTTON_DRAW = `.${CLASS_ANNOTATION_BUTTON_DRAW}`; export const CLASS_ANNOTATION_BUTTON_DRAW_ENTER = 'bp-btn-annotate-draw-enter'; export const SELECTOR_ANNOTATION_BUTTON_DRAW_ENTER = `.${CLASS_ANNOTATION_BUTTON_DRAW_ENTER}`; +export const CLASS_BOX_PREVIEW_HEADER_CONTAINER = 'bp-header-container'; +export const SELECTOR_BOX_PREVIEW_HEADER_CONTAINER = `.${CLASS_BOX_PREVIEW_HEADER_CONTAINER}`; export const CLASS_BOX_PREVIEW = 'bp'; export const SELECTOR_BOX_PREVIEW = `.${CLASS_BOX_PREVIEW}`; From 65aec89aa8ffc7533f0183ed95479989b7df8bcb Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 5 Oct 2018 16:09:41 -0700 Subject: [PATCH 5/6] Chore: addressing comments --- src/Annotator.js | 8 ++++---- src/controllers/AnnotationModeController.js | 4 ++++ .../__tests__/AnnotationModeController-test.js | 4 ++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Annotator.js b/src/Annotator.js index 97283f25d..98b79ddb3 100644 --- a/src/Annotator.js +++ b/src/Annotator.js @@ -104,18 +104,18 @@ class Annotator extends EventEmitter { // Get the container dom element if selector was passed, in tests this.container = this.options.container; if (typeof this.options.container === 'string') { - this.container = document.querySelector(this.options.container); + this.container = document.querySelector(this.container); } // Get the header dom element if selector was passed, in tests this.headerElement = this.options.headerElement; - if (typeof this.options.headerElement === 'string') { - this.headerElement = document.querySelector(this.options.headerElement); + if (typeof this.headerElement === 'string') { + this.headerElement = document.querySelector(this.headerElement); } // If using box content preview header and no external header element was specified, // fallback to the container element - if (this.options.header !== 'none' && !this.options.headerElement) { + if (this.options.header !== 'none' && !this.headerElement) { this.headerElement = this.container.querySelector(SELECTOR_BOX_PREVIEW_HEADER_CONTAINER); } diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 8ce685caf..4ba4d53ab 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -81,6 +81,10 @@ class AnnotationModeController extends EventEmitter { * @return {HTMLElement|null} Annotate button element or null if the selector did not find an element. */ getButton(annotatorSelector) { + if (!this.headerElement) { + return null; + } + return this.headerElement.querySelector(annotatorSelector); } diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index d40fb4e72..43f6fe093 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/src/controllers/__tests__/AnnotationModeController-test.js @@ -109,6 +109,10 @@ describe('controllers/AnnotationModeController', () => { expect(controller.getButton('.class')).not.toBeNull(); }); + + it('should return null if no headerElement', () => { + expect(controller.getButton('.class')).toBeNull(); + }); }); describe('showButton()', () => { From ecf0c9559435f654ac03775eec94f04e2cc14ef7 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 8 Oct 2018 14:07:16 -0700 Subject: [PATCH 6/6] Chore: Only hideButton if modeButton exists --- src/controllers/AnnotationModeController.js | 8 ++++--- .../AnnotationModeController-test.js | 23 ++++++++++++++++++- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 4ba4d53ab..612002936 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -71,7 +71,9 @@ class AnnotationModeController extends EventEmitter { this.buttonEl.removeEventListener('click', this.toggleMode); } - this.hideButton(); + if (this.modeButton) { + this.hideButton(); + } } /** @@ -94,7 +96,7 @@ class AnnotationModeController extends EventEmitter { * @return {void} */ showButton() { - if (!this.permissions.canAnnotate) { + if (!this.permissions.canAnnotate || !this.modeButton) { return; } @@ -114,7 +116,7 @@ class AnnotationModeController extends EventEmitter { * @return {void} */ hideButton() { - if (!this.permissions.canAnnotate) { + if (!this.permissions.canAnnotate || !this.modeButton) { return; } diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index 43f6fe093..39e740fce 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/src/controllers/__tests__/AnnotationModeController-test.js @@ -94,10 +94,17 @@ describe('controllers/AnnotationModeController', () => { expect(controller.buttonEl.removeEventListener).toBeCalled(); }); - it('should hide the button', () => { + it('should hide the button if modeButton exists', () => { + controller.modeButton = {}; controller.destroy(); expect(controller.hideButton).toBeCalled(); }); + + it('should not hide the button if modeButton does not exist', () => { + controller.modeButton = undefined; + controller.destroy(); + expect(controller.hideButton).not.toBeCalled(); + }); }); describe('getButton', () => { @@ -152,6 +159,13 @@ describe('controllers/AnnotationModeController', () => { expect(buttonEl.classList).not.toContain(CLASS_HIDDEN); expect(buttonEl.addEventListener).toBeCalledWith('click', controller.toggleMode); }); + + it('should do nothing if no modeButton', () => { + controller.modeButton = undefined; + controller.permissions.canAnnotate = false; + controller.showButton(); + expect(buttonEl.classList).toContain(CLASS_HIDDEN); + }); }); describe('hideButton()', () => { @@ -190,6 +204,13 @@ describe('controllers/AnnotationModeController', () => { controller.hideButton(); expect(buttonEl.classList).toContain(CLASS_HIDDEN); }); + + it('should do nothing if no modeButton', () => { + controller.modeButton = undefined; + controller.permissions.canAnnotate = false; + controller.hideButton(); + expect(buttonEl.classList).not.toContain(CLASS_HIDDEN); + }); }); describe('toggleMode()', () => {