Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update: Annotations with header #233

Merged
merged 6 commits into from Oct 11, 2018
Merged

Update: Annotations with header #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66ab1ed
Update: Adding headerElement to annotator options
ConradJChan Sep 28, 2018
3f1c482
Update: headerElement fallback to container when using preview header
ConradJChan Oct 1, 2018
5e75aa9
Chore: hide the annotation button on destroy
ConradJChan Oct 4, 2018
36417b0
Chore: default to header container
ConradJChan Oct 5, 2018
65aec89
Chore: addressing comments
ConradJChan Oct 5, 2018
ecf0c95
Chore: Only hideButton if modeButton exists
ConradJChan Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/Annotator.js
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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],
Expand Down
12 changes: 10 additions & 2 deletions src/__tests__/Annotator-test.js
View file
Open in desktop
Expand Up @@ -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 = `<button class="bp-btn-annotate"></button>
const html = `<div class="bp-header-container"></div>
<button class="bp-btn-annotate"></button>
<div class="annotated-element"></div>`;

describe('Annotator', () => {
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions src/constants.js
View file
Open in desktop
Expand Up @@ -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}`;
Expand Down
31 changes: 28 additions & 3 deletions src/controllers/AnnotationModeController.js
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -69,6 +70,10 @@ class AnnotationModeController extends EventEmitter {
if (this.buttonEl) {
this.buttonEl.removeEventListener('click', this.toggleMode);
}

if (this.modeButton) {
this.hideButton();
}
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -87,7 +96,7 @@ class AnnotationModeController extends EventEmitter {
* @return {void}
*/
showButton() {
if (!this.permissions.canAnnotate) {
if (!this.permissions.canAnnotate || !this.modeButton) {
return;
}

Expand All @@ -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.
Expand All @@ -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();

Expand Down
6 changes: 3 additions & 3 deletions src/controllers/DrawingModeController.js
View file
Open in desktop
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would this work in the cases where data.options.header !== 'none' and this.headerElement is null, would passsing in this.headerElement to the setupHeader method work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check in Annotator.js on line 117 is the attempt to handle this. If box content preview header is not none, and this.headerElement is null, then we fallback to this.container, so at this point in the DrawingModeController, this.header should be pointing at this.container

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

this.handleSelection = this.handleSelection.bind(this);
Expand Down Expand Up @@ -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);
}

Expand Down
6 changes: 3 additions & 3 deletions src/controllers/PointModeController.js
View file
Open in desktop
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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) {
Expand Down
78 changes: 75 additions & 3 deletions src/controllers/__tests__/AnnotationModeController-test.js
View file
Open in desktop
Expand Up @@ -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() };
Expand All @@ -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()', () => {
Expand Down Expand Up @@ -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()', () => {
Expand Down Expand Up @@ -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);
});
});

Expand Down
6 changes: 4 additions & 2 deletions src/controllers/__tests__/PointModeController-test.js
View file
Open in desktop
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
});

Expand Down