Skip to content

Commit

Permalink
fix(annotations): Fix cursor when drawing region in default mode (#1261)
Browse files Browse the repository at this point in the history 
* fix(annotations): Fix cursor when drawing region in default mode

* fix(annotations): Address comments and add tests

* fix(annotations): Address comments
  • Loading branch information
Mingze authored Sep 22, 2020
1 parent bf5bf17 commit 6b4f76d
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 32 deletions.
51 changes: 30 additions & 21 deletions src/lib/viewers/BaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class BaseViewer extends EventEmitter {
});
}

if (this.options.enableAnnotationsDiscoverability) {
if (this.options.enableAnnotationsDiscoverability && this.containerEl) {
this.containerEl.classList.add(CLASS_ANNOTATIONS_DISCOVERABLE);
}

Expand Down Expand Up @@ -1081,6 +1081,32 @@ class BaseViewer extends EventEmitter {
this.annotator.toggleAnnotationMode(AnnotationMode.NONE);
}

/**
* Handler for annotation mode change
* 1. Set annotationsControls mode
* 2. If discoverability FF is on, add mode classes to container
*
* @param {AnnotationMode} mode Next annotation mode
*/
processAnnotationModeChange = mode => {
if (!this.annotationControls) {
return;
}

this.annotationControls.setMode(mode);

if (this.options.enableAnnotationsDiscoverability && this.containerEl) {
switch (mode) {
case AnnotationMode.REGION:
this.containerEl.classList.add(CLASS_ANNOTATIONS_CREATE_REGION);
break;
default:
this.containerEl.classList.remove(CLASS_ANNOTATIONS_CREATE_REGION);
break;
}
}
};

/**
* Handler for annotation controls button click event.
*
Expand All @@ -1095,18 +1121,7 @@ class BaseViewer extends EventEmitter {
? AnnotationMode.REGION
: nextMode,
);
this.annotationControls.setMode(nextMode);

if (this.options.enableAnnotationsDiscoverability) {
switch (nextMode) {
case AnnotationMode.REGION:
this.containerEl.classList.add(CLASS_ANNOTATIONS_CREATE_REGION);
break;
default:
this.containerEl.classList.remove(CLASS_ANNOTATIONS_CREATE_REGION);
break;
}
}
this.processAnnotationModeChange(nextMode);
}

/**
Expand Down Expand Up @@ -1279,18 +1294,12 @@ class BaseViewer extends EventEmitter {
if (status === 'success') {
this.annotator.emit('annotations_active_set', id);

if (this.annotationControls) {
this.annotationControls.setMode(this.annotationControlsFSM.transition(AnnotationInput.SUCCESS));
}
this.processAnnotationModeChange(this.annotationControlsFSM.transition(AnnotationInput.SUCCESS));
}
}

handleAnnotationCreatorChangeEvent({ status, type }) {
if (!this.annotationControls) {
return;
}

this.annotationControls.setMode(this.annotationControlsFSM.transition(status, type));
this.processAnnotationModeChange(this.annotationControlsFSM.transition(status, type));
}

/**
Expand Down
46 changes: 35 additions & 11 deletions src/lib/viewers/__tests__/BaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1809,6 +1809,7 @@ describe('lib/viewers/BaseViewer', () => {
destroy: sandbox.stub(),
setMode: sandbox.stub(),
};
base.processAnnotationModeChange = sandbox.stub();
});

const createEvent = status => ({
Expand All @@ -1832,7 +1833,7 @@ describe('lib/viewers/BaseViewer', () => {
base.handleAnnotationCreateEvent(event);

expect(base.annotator.emit).to.be.calledWith('annotations_active_set', '123');
expect(base.annotationControls.setMode).to.be.calledWith(AnnotationMode.NONE);
expect(base.processAnnotationModeChange).to.be.calledWith(AnnotationMode.NONE);
});
});

Expand All @@ -1842,9 +1843,10 @@ describe('lib/viewers/BaseViewer', () => {
destroy: sandbox.stub(),
setMode: sandbox.stub(),
};
base.processAnnotationModeChange = sandbox.stub();
base.handleAnnotationCreatorChangeEvent({ status: AnnotationInput.CREATE, type: AnnotationMode.HIGHLIGHT });

expect(base.annotationControls.setMode).to.be.calledWith(AnnotationMode.HIGHLIGHT);
expect(base.processAnnotationModeChange).to.be.calledWith(AnnotationMode.HIGHLIGHT);
});
});

Expand All @@ -1862,21 +1864,17 @@ describe('lib/viewers/BaseViewer', () => {

describe('handleAnnotationControlsClick', () => {
beforeEach(() => {
base.annotationControls = {
destroy: sandbox.stub(),
setMode: sandbox.stub(),
};
base.containerEl = document.createElement('div');
base.annotator = {
toggleAnnotationMode: sandbox.stub(),
};
base.processAnnotationModeChange = sandbox.stub();
});

it('should call toggleAnnotationMode and setMode', () => {
it('should call toggleAnnotationMode and processAnnotationModeChange', () => {
base.handleAnnotationControlsClick({ mode: AnnotationMode.REGION });

expect(base.annotator.toggleAnnotationMode).to.be.calledWith(AnnotationMode.REGION);
expect(base.annotationControls.setMode).to.be.calledWith(AnnotationMode.REGION);
expect(base.processAnnotationModeChange).to.be.calledWith(AnnotationMode.REGION);
});

it('should call toggleAnnotationMode with appropriate mode if discoverability is enabled', () => {
Expand All @@ -1888,18 +1886,44 @@ describe('lib/viewers/BaseViewer', () => {
base.handleAnnotationControlsClick({ mode: AnnotationMode.NONE });
expect(base.annotator.toggleAnnotationMode).to.be.calledWith(AnnotationMode.REGION);
});
});

describe('processAnnotationModeChange()', () => {
beforeEach(() => {
base.annotationControls = {
destroy: sandbox.stub(),
setMode: sandbox.stub(),
};
base.containerEl = document.createElement('div');
});

it('should do nothing if no annotationControls', () => {
base.annotationControls = undefined;
sandbox.spy(base.containerEl.classList, 'add');
sandbox.spy(base.containerEl.classList, 'remove');
base.processAnnotationModeChange(AnnotationMode.REGION);

expect(base.containerEl.classList.add).to.not.be.called;
expect(base.containerEl.classList.remove).to.not.be.called;
});

it('should call annotationControls setMode', () => {
base.processAnnotationModeChange(AnnotationMode.REGION);

expect(base.annotationControls.setMode).to.be.calledWith(AnnotationMode.REGION);
});

it('should add create region class if discoverability is enabled and mode is REGION', () => {
base.options.enableAnnotationsDiscoverability = true;
base.handleAnnotationControlsClick({ mode: AnnotationMode.REGION });
base.processAnnotationModeChange(AnnotationMode.REGION);

expect(base.containerEl).to.have.class(CLASS_ANNOTATIONS_CREATE_REGION);
});

[AnnotationMode.NONE, AnnotationMode.HIGHLIGHT].forEach(mode => {
it(`should remove create region class if discoverability is enabled and mode is ${mode}`, () => {
base.options.enableAnnotationsDiscoverability = true;
base.handleAnnotationControlsClick({ mode });
base.processAnnotationModeChange(mode);

expect(base.containerEl).to.not.have.class(CLASS_ANNOTATIONS_CREATE_REGION);
});
Expand Down

0 comments on commit 6b4f76d

Please sign in to comment.