Skip to content

Commit

Permalink
fix(annotations): Fix region toggle button does not exit region mode (#…
Browse files Browse the repository at this point in the history 
…1228)

* fix(annotations): Fix region toggle button does not exit region mode

* fix(annotations): Address comments
  • Loading branch information
Mingze authored Jun 22, 2020
1 parent 94f062a commit e8b3cbb
Show file tree
Hide file tree
Showing 8 changed files with 49 additions and 52 deletions.
37 changes: 16 additions & 21 deletions src/lib/AnnotationControls.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ export enum AnnotationMode {
NONE = 'none',
REGION = 'region',
}
export type ClickHandler = ({ activeControl, event }: { activeControl: AnnotationMode; event: MouseEvent }) => void;
export type ClickHandler = ({ event }: { event: MouseEvent }) => void;
export type Options = {
onEscape?: () => void;
onRegionClick?: ClickHandler;
onReset?: () => void;
};

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

private controlsMap: ControlsMap;

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

private hasInit = false;

private onReset: () => void = noop;
private onEscape: () => void = noop;

/**
* [constructor]
Expand Down Expand Up @@ -105,24 +105,18 @@ export default class AnnotationControls {
*/
private handleFullscreenExit = (): void => this.handleFullscreenChange(false);

public getActiveMode = (): AnnotationMode => {
return this.currentActiveControl;
};

/**
* Deactivate current control button
*/
public resetControls = (): void => {
if (this.currentActiveControl === AnnotationMode.NONE) {
if (this.currentMode === AnnotationMode.NONE) {
return;
}

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

this.currentActiveControl = AnnotationMode.NONE;
this.currentMode = AnnotationMode.NONE;
updateButton();

this.onReset();
};

/**
Expand All @@ -135,7 +129,7 @@ export default class AnnotationControls {
return;
}

if (this.currentActiveControl === AnnotationMode.REGION) {
if (this.currentMode === AnnotationMode.REGION) {
regionButtonElement.classList.add(CLASS_BUTTON_ACTIVE);
} else {
regionButtonElement.classList.remove(CLASS_BUTTON_ACTIVE);
Expand All @@ -146,28 +140,29 @@ export default class AnnotationControls {
* Region comment button click handler
*/
private handleClick = (onClick: ClickHandler, mode: AnnotationMode) => (event: MouseEvent): void => {
const prevActiveControl = this.currentActiveControl;
const prevMode = this.currentMode;

this.resetControls();

if (prevActiveControl !== mode) {
this.currentActiveControl = mode as AnnotationMode;
if (prevMode !== mode) {
this.currentMode = mode as AnnotationMode;
this.controlsMap[mode]();
}

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

/**
* Escape key handler, reset all control buttons,
* and stop propagation to prevent preview modal from exiting
*/
private handleKeyDown = (event: KeyboardEvent): void => {
if (event.key !== 'Escape' || this.currentActiveControl === AnnotationMode.NONE) {
if (event.key !== 'Escape' || this.currentMode === AnnotationMode.NONE) {
return;
}

this.resetControls();
this.onEscape();

event.preventDefault();
event.stopPropagation();
Expand All @@ -176,7 +171,7 @@ export default class AnnotationControls {
/**
* Initialize the annotation controls with options.
*/
public init({ onRegionClick = noop, onReset = noop }: Options = {}): void {
public init({ onEscape = noop, onRegionClick = noop }: Options = {}): void {
if (this.hasInit) {
return;
}
Expand All @@ -192,7 +187,7 @@ export default class AnnotationControls {

regionButton.setAttribute('data-testid', 'bp-AnnotationsControls-regionBtn');

this.onReset = onReset;
this.onEscape = onEscape;
document.addEventListener('keydown', this.handleKeyDown);

this.hasInit = true;
Expand Down
36 changes: 13 additions & 23 deletions src/lib/__tests__/AnnotationControls-test.js
View file
Original file line number Diff line number Diff line change
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.equal(AnnotationMode.NONE);
expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE);
});

it('should attach event listeners', () => {
Expand Down Expand Up @@ -119,11 +119,11 @@ describe('lib/AnnotationControls', () => {
});

it('should set onRest and hasInit', () => {
const onResetMock = sandbox.stub();
const onEscapeMock = sandbox.stub();

annotationControls.init({ onReset: onResetMock });
annotationControls.init({ onEscape: onEscapeMock });

expect(annotationControls.onReset).to.equal(onResetMock);
expect(annotationControls.onEscape).to.equal(onEscapeMock);
expect(annotationControls.hasInit).to.equal(true);
});

Expand All @@ -144,7 +144,7 @@ describe('lib/AnnotationControls', () => {

beforeEach(() => {
annotationControls.resetControls = sandbox.stub();
annotationControls.currentActiveControl = 'region';
annotationControls.currentMode = 'region';
eventMock = {
key: 'Escape',
preventDefault: sandbox.stub(),
Expand All @@ -157,7 +157,7 @@ describe('lib/AnnotationControls', () => {

expect(annotationControls.resetControls).not.to.be.called;

annotationControls.currentActiveControl = 'none';
annotationControls.currentMode = 'none';
annotationControls.handleKeyDown({ key: 'Escape' });

expect(annotationControls.resetControls).not.to.be.called;
Expand All @@ -177,22 +177,21 @@ describe('lib/AnnotationControls', () => {
});

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

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

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

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

expect(stubs.onRegionClick).to.be.calledWith({
activeControl: AnnotationMode.REGION,
event: stubs.event,
});
});
Expand All @@ -213,35 +212,26 @@ describe('lib/AnnotationControls', () => {
describe('resetControls()', () => {
beforeEach(() => {
stubs.updateRegionButton = sandbox.stub();
stubs.onReset = sandbox.stub();
stubs.onEscape = sandbox.stub();
annotationControls.controlsMap = {
[AnnotationMode.REGION]: stubs.updateRegionButton,
};
annotationControls.onReset = stubs.onReset;
});

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

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

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

annotationControls.resetControls();

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

describe('getActiveMode()', () => {
it('should return the current active mode', () => {
annotationControls.currentActiveControl = 'region';
expect(annotationControls.getActiveMode()).to.equal('region');
});
});
});
4 changes: 2 additions & 2 deletions src/lib/viewers/BaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class BaseViewer extends EventEmitter {
this.mobileZoomEndHandler = this.mobileZoomEndHandler.bind(this);
this.handleAnnotatorEvents = this.handleAnnotatorEvents.bind(this);
this.handleAnnotationCreateEvent = this.handleAnnotationCreateEvent.bind(this);
this.handleAnnotationControlsReset = this.handleAnnotationControlsReset.bind(this);
this.handleAnnotationControlsEscape = this.handleAnnotationControlsEscape.bind(this);
this.handleFullscreenEnter = this.handleFullscreenEnter.bind(this);
this.handleFullscreenExit = this.handleFullscreenExit.bind(this);
this.handleRegionClick = this.handleRegionClick.bind(this);
Expand Down Expand Up @@ -1047,7 +1047,7 @@ class BaseViewer extends EventEmitter {
* @private
* @return {void}
*/
handleAnnotationControlsReset() {
handleAnnotationControlsEscape() {
this.annotator.toggleAnnotationMode(AnnotationMode.NONE);
}

Expand Down
16 changes: 14 additions & 2 deletions src/lib/viewers/__tests__/BaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1749,15 +1749,27 @@ describe('lib/viewers/BaseViewer', () => {
});
});

describe('handleAnnotationControlsReset()', () => {
describe('handleAnnotationControlsEscape()', () => {
it('should call toggleAnnotationMode', () => {
base.annotator = {
toggleAnnotationMode: sandbox.stub(),
};

base.handleAnnotationControlsReset();
base.handleAnnotationControlsEscape();

expect(base.annotator.toggleAnnotationMode).to.be.calledWith('none');
});
});

describe('handleRegionClick', () => {
it('should call toggleAnnotationMode', () => {
base.annotator = {
toggleAnnotationMode: sandbox.stub(),
};

base.handleRegionClick();

expect(base.annotator.toggleAnnotationMode).to.be.calledWith('region');
});
});
});
2 changes: 1 addition & 1 deletion src/lib/viewers/doc/DocBaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1099,8 +1099,8 @@ class DocBaseViewer extends BaseViewer {

if (this.areNewAnnotationsEnabled() && this.hasAnnotationCreatePermission()) {
this.annotationControls.init({
onEscape: this.handleAnnotationControlsEscape,
onRegionClick: this.handleRegionClick,
onReset: this.handleAnnotationControlsReset,
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2303,8 +2303,8 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
ICON_FULLSCREEN_OUT,
);
expect(docBase.annotationControls.init).to.be.calledWith({
onEscape: docBase.handleAnnotationControlsEscape,
onRegionClick: docBase.handleRegionClick,
onReset: docBase.handleAnnotationControlsReset,
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/lib/viewers/image/ImageViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,8 @@ class ImageViewer extends ImageBaseViewer {
if (this.areNewAnnotationsEnabled() && this.hasAnnotationCreatePermission()) {
this.annotationControls = new AnnotationControls(this.controls);
this.annotationControls.init({
onEscape: this.handleAnnotationControlsEscape,
onRegionClick: this.handleRegionClick,
onReset: this.handleAnnotationControlsReset,
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/viewers/image/__tests__/ImageViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,8 @@ describe('lib/viewers/image/ImageViewer', () => {
image.loadUI();

expect(AnnotationControls.prototype.init).to.be.calledWith({
onEscape: image.handleAnnotationControlsEscape,
onRegionClick: image.handleRegionClick,
onReset: image.handleAnnotationControlsReset,
});
});
});
Expand Down

0 comments on commit e8b3cbb

Please sign in to comment.