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

feat(annotations): Toggle region annotation mode #1192

Merged
merged 7 commits into from
Apr 2, 2020
Merged

feat(annotations): Toggle region annotation mode #1192

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e6f477
feat(annotations): Toggle region annotation mode
Apr 1, 2020
7101a7f
feat(annotations): Move addListener to BaseViewer
Apr 1, 2020
06ee4db
feat(annotations): Address comments
Apr 2, 2020
3c1637a
feat(annotations): Update tests
Apr 2, 2020
acb4e32
feat(annotations): Address comments
Apr 2, 2020
6b8f18c
feat(annotations): Address comments
Apr 2, 2020
2196c13
Merge branch 'master' into toggle-mode
Apr 2, 2020
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
52 changes: 28 additions & 24 deletions src/lib/AnnotationControls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
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 @@ -11,10 +10,13 @@ export const CLASS_REGION_BUTTON = 'bp-AnnotationControls-regionBtn';
export const CLASS_BUTTON_ACTIVE = 'is-active';
export const CLASS_GROUP_HIDE = 'is-hidden';

export type AnnotationMode = 'draw' | 'highlight' | 'region' | null;
export type RegionHandler = ({ activeControl, event }: { activeControl: AnnotationMode; event: MouseEvent }) => void;
export enum AnnotationMode {
NONE = 'none',
REGION = 'region',
}
export type ClickHandler = ({ activeControl, event }: { activeControl: AnnotationMode; event: MouseEvent }) => void;
export type Options = {
onRegionClick?: RegionHandler;
onRegionClick?: ClickHandler;
};

declare const __: (key: string) => string;
Expand All @@ -30,7 +32,7 @@ export default class AnnotationControls {

private controlsMap: ControlsMap;

private currentActiveControl: AnnotationMode = null;
private currentActiveControl: AnnotationMode = AnnotationMode.NONE;

/**
* [constructor]
Expand All @@ -43,7 +45,7 @@ export default class AnnotationControls {
this.controls = controls;
this.controlsElement = controls.controlsEl;
this.controlsMap = {
[ANNOTATION_MODE.region]: this.updateRegionButton,
[AnnotationMode.REGION]: this.updateRegionButton,
};

this.attachEventHandlers();
Expand Down Expand Up @@ -93,16 +95,16 @@ export default class AnnotationControls {
private handleFullscreenExit = (): void => this.handleFullscreenChange(false);

/**
* Deactive current control button
* Deactivate current control button
Copy link
Contributor

Choose a reason for hiding this comment

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

Update method description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update method description?

I think it's the same meaning 😂

*/
public deactivateCurrentControl = (): void => {
if (!this.currentActiveControl) {
public resetControls = (): void => {
if (this.currentActiveControl === AnnotationMode.NONE) {
return;
}

const updateButton = this.controlsMap[this.currentActiveControl];

this.currentActiveControl = null;
this.currentActiveControl = AnnotationMode.NONE;
updateButton();
};

Expand All @@ -112,29 +114,31 @@ export default class AnnotationControls {
private updateRegionButton = (): void => {
const regionButtonElement = this.controlsElement.querySelector(`.${CLASS_REGION_BUTTON}`);

if (regionButtonElement) {
if (this.currentActiveControl === ANNOTATION_MODE.region) {
regionButtonElement.classList.add(CLASS_BUTTON_ACTIVE);
} else {
regionButtonElement.classList.remove(CLASS_BUTTON_ACTIVE);
}
if (!regionButtonElement) {
return;
}

if (this.currentActiveControl === AnnotationMode.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 => {
const activeControl = this.currentActiveControl;
private handleClick = (onClick: ClickHandler, mode: AnnotationMode) => (event: MouseEvent): void => {
const prevActiveControl = this.currentActiveControl;

this.deactivateCurrentControl();
this.resetControls();

if (activeControl !== ANNOTATION_MODE.region) {
this.currentActiveControl = ANNOTATION_MODE.region as AnnotationMode;
this.updateRegionButton();
if (prevActiveControl !== mode) {
this.currentActiveControl = mode as AnnotationMode;
this.controlsMap[mode]();
}

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

/**
Expand All @@ -144,7 +148,7 @@ export default class AnnotationControls {
const groupElement = this.controls.addGroup(CLASS_ANNOTATIONS_GROUP);
this.controls.add(
__('region_comment'),
this.handleRegionClick(onRegionClick),
this.handleClick(onRegionClick, AnnotationMode.REGION),
`${CLASS_BOX_CONTROLS_GROUP_BUTTON} ${CLASS_REGION_BUTTON}`,
ICON_REGION_COMMENT,
'button',
Expand Down
36 changes: 18 additions & 18 deletions src/lib/__tests__/AnnotationControls-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable no-unused-expressions */
import { ANNOTATION_MODE } from '../constants';
import { ICON_REGION_COMMENT } from '../icons/icons';
import AnnotationControls, {
AnnotationMode,
CLASS_ANNOTATIONS_GROUP,
CLASS_BUTTON_ACTIVE,
CLASS_GROUP_HIDE,
Expand Down Expand Up @@ -52,7 +52,7 @@ describe('lib/AnnotationControls', () => {
expect(annotationControls.controls).not.to.be.undefined;
expect(annotationControls.controlsElement).not.to.be.undefined;
expect(annotationControls.controlsMap).not.to.be.undefined;
expect(annotationControls.currentActiveControl).to.be.null;
expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE);
});

it('should attach event listeners', () => {
Expand All @@ -76,7 +76,7 @@ describe('lib/AnnotationControls', () => {
beforeEach(() => {
stubs.add = sandbox.stub(annotationControls.controls, 'add');
stubs.regionHandler = sandbox.stub();
sandbox.stub(annotationControls, 'handleRegionClick').returns(stubs.regionHandler);
sandbox.stub(annotationControls, 'handleClick').returns(stubs.regionHandler);
});

it('should add the controls', () => {
Expand All @@ -93,28 +93,28 @@ describe('lib/AnnotationControls', () => {
});
});

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

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

annotationControls.handleRegionClick(stubs.onRegionClick)(stubs.event);
expect(annotationControls.currentActiveControl).to.equal(ANNOTATION_MODE.region);
annotationControls.handleClick(stubs.onRegionClick, AnnotationMode.REGION)(stubs.event);
expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.REGION);
expect(stubs.classListAdd).to.be.calledWith(CLASS_BUTTON_ACTIVE);

annotationControls.handleRegionClick(stubs.onRegionClick)(stubs.event);
expect(annotationControls.currentActiveControl).to.equal(null);
annotationControls.handleClick(stubs.onRegionClick, AnnotationMode.REGION)(stubs.event);
expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE);
expect(stubs.classListRemove).to.be.calledWith(CLASS_BUTTON_ACTIVE);
});

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

expect(stubs.onRegionClick).to.be.calledWith({
activeControl: ANNOTATION_MODE.region,
activeControl: AnnotationMode.REGION,
event: stubs.event,
});
});
Expand All @@ -132,27 +132,27 @@ describe('lib/AnnotationControls', () => {
});
});

describe('deactivateCurrentControl()', () => {
describe('resetControls()', () => {
beforeEach(() => {
stubs.updateRegionButton = sandbox.stub();
annotationControls.controlsMap = {
[ANNOTATION_MODE.region]: stubs.updateRegionButton,
[AnnotationMode.REGION]: stubs.updateRegionButton,
};
});

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

expect(annotationControls.currentActiveControl).to.be.null;
expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE);
expect(stubs.updateRegionButton).not.to.be.called;
});

it('should call updateRegionButton if current control is region', () => {
annotationControls.currentActiveControl = ANNOTATION_MODE.region;
annotationControls.currentActiveControl = AnnotationMode.REGION;

annotationControls.deactivateCurrentControl();
annotationControls.resetControls();

expect(annotationControls.currentActiveControl).to.be.null;
expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE);
expect(stubs.updateRegionButton).to.be.called;
});
});
Expand Down
6 changes: 0 additions & 6 deletions src/lib/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,6 @@ 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
2 changes: 1 addition & 1 deletion src/lib/viewers/BaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ class BaseViewer extends EventEmitter {
this.annotator.addListener('annotatorevent', this.handleAnnotatorEvents);

if (this.options.showAnnotationsControls && this.annotationControls) {
this.annotator.addListener('annotationcreate', this.annotationControls.deactivateCurrentControl);
this.annotator.addListener('annotationcreate', this.annotationControls.resetControls);
}
}

Expand Down
7 changes: 5 additions & 2 deletions src/lib/viewers/__tests__/BaseViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ describe('lib/viewers/BaseViewer', () => {
CONSTRUCTOR: sandbox.stub().returns(base.annotator),
};
base.annotationControls = {
deactivateCurrentControl: sandbox.stub(),
resetControls: sandbox.stub(),
};
});

Expand All @@ -1122,7 +1122,10 @@ describe('lib/viewers/BaseViewer', () => {
expect(base.addListener).to.be.calledWith('toggleannotationmode', sinon.match.func);
expect(base.addListener).to.be.calledWith('scale', sinon.match.func);
expect(base.addListener).to.be.calledWith('scrolltoannotation', sinon.match.func);
expect(base.annotator.addListener).to.be.calledWith('annotationcreate', sinon.match.func);
expect(base.annotator.addListener).to.be.calledWith(
'annotationcreate',
base.annotationControls.resetControls,
);
expect(base.annotator.addListener).to.be.calledWith('annotatorevent', sinon.match.func);
expect(base.emit).to.be.calledWith('annotator', base.annotator);
});
Expand Down
6 changes: 3 additions & 3 deletions src/lib/viewers/doc/DocBaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import throttle from 'lodash/throttle';
import AnnotationControls from '../../AnnotationControls';
import AnnotationControls, { AnnotationMode } from '../../AnnotationControls';
import BaseViewer from '../BaseViewer';
import Browser from '../../Browser';
import Controls from '../../Controls';
Expand Down Expand Up @@ -1113,8 +1113,8 @@ class DocBaseViewer extends BaseViewer {
* @private
* @return {void}
*/
regionClickHandler({ activeControl }) {
this.annotator.toggleAnnotationMode(activeControl);
regionClickHandler() {
this.annotator.toggleAnnotationMode(AnnotationMode.REGION);
}

/**
Expand Down