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.

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

feat(annotations): Toggle region annotation mode #1192

Merged
merged 7 commits into from
Apr 2, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 49 additions & 11 deletions src/lib/AnnotationControls.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { EventEmitter } from 'events';
import noop from 'lodash/noop';

import { ANNOTATION_MODE } from './constants';
import { ICON_REGION_COMMENT } from './icons/icons';
import Controls, { CLASS_BOX_CONTROLS_GROUP_BUTTON } from './Controls';
import fullscreen from './Fullscreen';
Expand All @@ -9,31 +12,32 @@ export const CLASS_REGION_BUTTON = 'bp-AnnotationControls-regionBtn';
export const CLASS_BUTTON_ACTIVE = 'is-active';
export const CLASS_GROUP_HIDE = 'is-hidden';

export type RegionHandler = ({ isRegionActive, event }: { isRegionActive: boolean; event: MouseEvent }) => void;
export type AnnotationMode = 'draw' | 'highlight' | 'region' | null;
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
export type RegionHandler = ({ activeControl, event }: { activeControl: AnnotationMode; event: MouseEvent }) => void;
export type Options = {
onRegionClick?: RegionHandler;
};

declare const __: (key: string) => string;

export default class AnnotationControls {
/** @property {Controls} - Controls object */
private annotator: EventEmitter;

private controls: Controls;

/** @property {HTMLElement} - Controls element */
private controlsElement: HTMLElement;

/** @property {boolean} - Region comment mode active state */
private isRegionActive = false;
private currentActiveControl: AnnotationMode = null;

/**
* [constructor]
*/
constructor(controls: Controls) {
constructor(controls: Controls, annotator: EventEmitter) {
if (!controls || !(controls instanceof Controls)) {
throw Error('controls must be an instance of Controls');
}

this.annotator = annotator;
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
this.controls = controls;
this.controlsElement = controls.controlsEl;

Expand All @@ -46,6 +50,7 @@ export default class AnnotationControls {
public destroy(): void {
fullscreen.removeListener('enter', this.handleFullscreenEnter);
fullscreen.removeListener('exit', this.handleFullscreenExit);
this.annotator.removeListener('annotationcreate', this.deactivateCurrentControl);
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -54,6 +59,7 @@ export default class AnnotationControls {
private attachEventHandlers(): void {
fullscreen.addListener('enter', this.handleFullscreenEnter);
fullscreen.addListener('exit', this.handleFullscreenExit);
this.annotator.addListener('annotationcreate', this.deactivateCurrentControl);
}

/**
Expand Down Expand Up @@ -84,21 +90,53 @@ export default class AnnotationControls {
private handleFullscreenExit = (): void => this.handleFullscreenChange(false);

/**
* Region comment button click handler
* Deactive current control button
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
*/
private handleRegionClick = (onRegionClick: RegionHandler) => (event: MouseEvent): void => {
public deactivateCurrentControl = (): void => {
if (!this.currentActiveControl) {
return;
}

switch (this.currentActiveControl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the switch is needed at all. Should it just blindly run through all the update*Button methods each time? For instance if the click region and then click highlight, we would need region and highlight to update respectively

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if the switch is needed at all. Should it just blindly run through all the update*Button methods each time? For instance if the click region and then click highlight, we would need region and highlight to update respectively

This is deactivateCurrentControl(), which only deactivate the current control. Update next control will happen in handle*Click.

case ANNOTATION_MODE.region:
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
this.currentActiveControl = null;
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
this.updateRegionButton();
break;
default:
this.currentActiveControl = null;
}
};

/**
* Update region button UI
*/
private updateRegionButton = (): void => {
const regionButtonElement = this.controlsElement.querySelector(`.${CLASS_REGION_BUTTON}`);

if (regionButtonElement) {
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
this.isRegionActive = !this.isRegionActive;
if (this.isRegionActive) {
if (this.currentActiveControl === ANNOTATION_MODE.region) {
regionButtonElement.classList.add(CLASS_BUTTON_ACTIVE);
} else {
regionButtonElement.classList.remove(CLASS_BUTTON_ACTIVE);
}
}
};

/**
* Region comment button click handler
*/
private handleRegionClick = (onRegionClick: RegionHandler) => (event: MouseEvent): void => {
if (this.currentActiveControl === ANNOTATION_MODE.region) {
this.deactivateCurrentControl();
} else {
if (this.currentActiveControl) {
this.deactivateCurrentControl();
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved
}
this.currentActiveControl = ANNOTATION_MODE.region as AnnotationMode;
this.updateRegionButton();
}

onRegionClick({ isRegionActive: this.isRegionActive, event });
onRegionClick({ activeControl: this.currentActiveControl, event });
};

/**
Expand Down
47 changes: 40 additions & 7 deletions src/lib/__tests__/AnnotationControls-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* eslint-disable no-unused-expressions */
import { ANNOTATION_MODE } from '../constants';
import { ICON_REGION_COMMENT } from '../icons/icons';
import AnnotationControls, {
CLASS_ANNOTATIONS_GROUP,
Expand All @@ -21,8 +22,8 @@ describe('lib/AnnotationControls', () => {

beforeEach(() => {
fixture.load('__tests__/AnnotationControls-test.html');
const controls = new Controls(document.getElementById('test-annotation-controls-container'));

stubs.annotatorAddListener = sandbox.stub();
stubs.annotatorRemoveListener = sandbox.stub();
stubs.classListAdd = sandbox.stub();
stubs.classListRemove = sandbox.stub();
stubs.fullscreenAddListener = sandbox.stub(fullscreen, 'addListener');
Expand All @@ -34,7 +35,12 @@ describe('lib/AnnotationControls', () => {
},
});

annotationControls = new AnnotationControls(controls);
const controls = new Controls(document.getElementById('test-annotation-controls-container'));
const annotator = {
addListener: stubs.annotatorAddListener,
removeListener: stubs.annotatorRemoveListener,
};
annotationControls = new AnnotationControls(controls, annotator);
annotationControls.controlsElement.querySelector = stubs.querySelector;
});

Expand All @@ -48,11 +54,15 @@ describe('lib/AnnotationControls', () => {

describe('constructor()', () => {
it('should create the correct DOM structure', () => {
expect(annotationControls.annotator).not.to.be.undefined;
expect(annotationControls.controls).not.to.be.undefined;
expect(annotationControls.controlsElement).not.to.be.undefined;
expect(annotationControls.currentActiveControl).to.be.null;
});

it('should attach event listeners', () => {
expect(stubs.fullscreenAddListener).to.be.calledTwice;
expect(stubs.annotatorAddListener).to.be.called;
});

it('should throw an exception if controls is not provided', () => {
Expand All @@ -69,6 +79,7 @@ describe('lib/AnnotationControls', () => {
annotationControls.destroy();

expect(stubs.fullscreenRemoveListener).to.be.calledTwice;
expect(stubs.annotatorRemoveListener).to.be.called;
});
});

Expand All @@ -94,23 +105,27 @@ describe('lib/AnnotationControls', () => {
});

describe('handleRegionClick()', () => {
beforeEach(() => {
stubs.event = sandbox.stub({});
});

it('should activate region button then deactivate', () => {
expect(annotationControls.isRegionActive).to.be.false;
expect(annotationControls.currentActiveControl).to.be.null;

annotationControls.handleRegionClick(stubs.onRegionClick)(stubs.event);
expect(annotationControls.isRegionActive).to.be.true;
expect(annotationControls.currentActiveControl).to.equal(ANNOTATION_MODE.region);
expect(stubs.classListAdd).to.be.calledWith(CLASS_BUTTON_ACTIVE);

annotationControls.handleRegionClick(stubs.onRegionClick)(stubs.event);
expect(annotationControls.isRegionActive).to.be.false;
expect(annotationControls.currentActiveControl).to.equal(null);
expect(stubs.classListRemove).to.be.calledWith(CLASS_BUTTON_ACTIVE);
});

it('should call onRegionClick', () => {
annotationControls.handleRegionClick(stubs.onRegionClick)(stubs.event);

expect(stubs.onRegionClick).to.be.calledWith({
isRegionActive: true,
activeControl: ANNOTATION_MODE.region,
event: stubs.event,
});
});
Expand All @@ -127,4 +142,22 @@ describe('lib/AnnotationControls', () => {
expect(stubs.classListRemove).to.be.calledWith(CLASS_GROUP_HIDE);
});
});

describe('deactivateCurrentControl()', () => {
it('should not change if no current active control', () => {
annotationControls.deactivateCurrentControl();

expect(annotationControls.currentActiveControl).to.be.null;
});

it('should call updateRegionButton if current control is region', () => {
stubs.updateRegion = sandbox.stub(annotationControls, 'updateRegionButton');
annotationControls.currentActiveControl = ANNOTATION_MODE.region;

annotationControls.deactivateCurrentControl();

expect(annotationControls.currentActiveControl).to.be.null;
expect(stubs.updateRegion).to.be.called;
});
});
});
6 changes: 6 additions & 0 deletions src/lib/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ export const ANNOTATOR_EVENT = {
scale: 'scaleannotations',
};

export const ANNOTATION_MODE = {
draw: 'draw',
highlight: 'highlight',
region: 'region',
};

export const BROWSERS = {
CHROME: 'Chrome',
EDGE: 'Edge',
Expand Down
6 changes: 4 additions & 2 deletions src/lib/viewers/doc/DocBaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ class DocBaseViewer extends BaseViewer {
this.pageControls = new PageControls(this.controls, this.docEl);
this.zoomControls = new ZoomControls(this.controls);
if (this.options.showAnnotationsControls) {
this.annotationControls = new AnnotationControls(this.controls);
this.annotationControls = new AnnotationControls(this.controls, this.annotator);
}
this.pageControls.addListener('pagechange', this.setPage);
this.bindControlListeners();
Expand Down Expand Up @@ -1113,7 +1113,9 @@ class DocBaseViewer extends BaseViewer {
* @private
* @return {void}
*/
regionClickHandler() {}
regionClickHandler({ activeControl }) {
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
this.annotator.toggleAnnotationMode(activeControl);
}

/**
* Handler for 'pagesinit' event.
Expand Down