From 91cdce390f564e14d65c635b4812c1dd4ea18296 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 11 Oct 2018 10:25:50 -0700 Subject: [PATCH] Update: Annotations with header (#233) * Update: Adding headerElement to annotator options * Update: headerElement fallback to container when using preview header * Chore: hide the annotation button on destroy * Chore: default to header container * Chore: addressing comments * Chore: Only hideButton if modeButton exists --- src/Annotator.js | 18 ++++- src/__tests__/Annotator-test.js | 12 ++- src/constants.js | 2 + src/controllers/AnnotationModeController.js | 31 +++++++- src/controllers/DrawingModeController.js | 6 +- src/controllers/PointModeController.js | 6 +- .../AnnotationModeController-test.js | 78 ++++++++++++++++++- .../__tests__/PointModeController-test.js | 6 +- 8 files changed, 141 insertions(+), 18 deletions(-) diff --git a/src/Annotator.js b/src/Annotator.js index 464324412..98b79ddb3 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 { @@ -103,7 +104,19 @@ 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.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.headerElement) { + this.headerElement = this.container.querySelector(SELECTOR_BOX_PREVIEW_HEADER_CONTAINER); } // Get annotated element from container @@ -243,6 +256,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/__tests__/Annotator-test.js b/src/__tests__/Annotator-test.js index 2daa793e4..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', () => { @@ -112,6 +114,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.querySelector(SELECTOR_BOX_PREVIEW_HEADER_CONTAINER)); + }); + it('should setup mobile dialog for mobile browsers', () => { annotator.isMobile = true; annotator.init(); 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}`; diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 07eab09d5..612002936 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; @@ -69,6 +70,10 @@ class AnnotationModeController extends EventEmitter { if (this.buttonEl) { this.buttonEl.removeEventListener('click', this.toggleMode); } + + if (this.modeButton) { + this.hideButton(); + } } /** @@ -78,7 +83,11 @@ 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); + if (!this.headerElement) { + return null; + } + + return this.headerElement.querySelector(annotatorSelector); } /** @@ -87,7 +96,7 @@ class AnnotationModeController extends EventEmitter { * @return {void} */ showButton() { - if (!this.permissions.canAnnotate) { + if (!this.permissions.canAnnotate || !this.modeButton) { return; } @@ -101,6 +110,22 @@ class AnnotationModeController extends EventEmitter { } } + /** + * Hides the annotate button for the specified mode + * + * @return {void} + */ + hideButton() { + if (!this.permissions.canAnnotate || !this.modeButton) { + 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. @@ -126,7 +151,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..39e740fce 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,17 +93,33 @@ describe('controllers/AnnotationModeController', () => { controller.destroy(); expect(controller.buttonEl.removeEventListener).toBeCalled(); }); + + 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', () => { 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(); }); + + it('should return null if no headerElement', () => { + expect(controller.getButton('.class')).toBeNull(); + }); }); describe('showButton()', () => { @@ -139,6 +159,58 @@ 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()', () => { + 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); + }); + + 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()', () => { @@ -178,7 +250,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); }); });