Skip to content

Commit

Permalink
Fix: Cleanup hiding/closing of the mobile dialog (#141)
Browse files Browse the repository at this point in the history 
- Simplified some of the logic triggered by closing either mobile point or highlight dialogs. 
- Fixes issue where the mobile highlight comment dialog didn't close on clicking the 'X' button
  • Loading branch information
@pramodsum
pramodsum authored Mar 20, 2018
1 parent b0afeb4 commit 99ccc22
Show file tree
Hide file tree
Showing 13 changed files with 109 additions and 123 deletions.
45 changes: 8 additions & 37 deletions src/AnnotationDialog.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import EventEmitter from 'events';
import * as util from './util';
import * as constants from './constants';
import { ICON_CLOSE, ICON_DELETE } from './icons/icons';
import { ICON_DELETE } from './icons/icons';

const POINT_ANNOTATION_ICON_HEIGHT = 31;
const POINT_ANNOTATION_ICON_DOT_HEIGHT = 8;
Expand Down Expand Up @@ -63,8 +63,6 @@ class AnnotationDialog extends EventEmitter {
if (!this.isMobile) {
this.mouseenterHandler = this.mouseenterHandler.bind(this);
this.mouseleaveHandler = this.mouseleaveHandler.bind(this);
} else {
this.hideMobileDialog = this.hideMobileDialog.bind(this);
}
}

Expand Down Expand Up @@ -180,7 +178,7 @@ class AnnotationDialog extends EventEmitter {
}

/**
* Hides and resets the shared mobile dialog.
* Hides the shared mobile dialog.
*
* @return {void}
*/
Expand All @@ -193,16 +191,9 @@ class AnnotationDialog extends EventEmitter {
this.dialogEl.parentNode.removeChild(this.dialogEl);
}

this.element.classList.remove(constants.CLASS_ANIMATE_DIALOG);

// Clear annotations from dialog
this.element.innerHTML = `
<div class="${constants.CLASS_MOBILE_DIALOG_HEADER}">
<button class="${constants.CLASS_DIALOG_CLOSE}">${ICON_CLOSE}</button>
</div>`.trim();
this.element.classList.remove(constants.CLASS_ANNOTATION_PLAIN_HIGHLIGHT);

util.hideElement(this.element);
this.element = util.generateMobileDialogEl();
this.unbindDOMListeners();

// Cancel any unsaved annotations
Expand Down Expand Up @@ -387,14 +378,6 @@ class AnnotationDialog extends EventEmitter {
this.element.addEventListener('click', this.clickHandler);
this.element.addEventListener('mouseenter', this.mouseenterHandler);
this.element.addEventListener('mouseleave', this.mouseleaveHandler);
return;
}

const dialogCloseButtonEl = this.element.querySelector(constants.SELECTOR_DIALOG_CLOSE);
dialogCloseButtonEl.addEventListener('click', this.hideMobileDialog);

if (this.hasTouch) {
dialogCloseButtonEl.addEventListener('touchstart', this.hideMobileDialog);
}
}

Expand Down Expand Up @@ -444,14 +427,6 @@ class AnnotationDialog extends EventEmitter {
this.element.removeEventListener('click', this.clickHandler);
this.element.removeEventListener('mouseenter', this.mouseenterHandler);
this.element.removeEventListener('mouseleave', this.mouseleaveHandler);
return;
}

const dialogCloseButtonEl = this.element.querySelector(constants.SELECTOR_DIALOG_CLOSE);
dialogCloseButtonEl.removeEventListener('click', this.hideMobileDialog);

if (this.hasTouch) {
dialogCloseButtonEl.removeEventListener('touchstart', this.hideMobileDialog);
}
}

Expand Down Expand Up @@ -590,14 +565,11 @@ class AnnotationDialog extends EventEmitter {
case constants.DATA_TYPE_POST:
this.postAnnotation();
break;
// Clicking 'Cancel' button to cancel the annotation
// Clicking 'Cancel' button to cancel the annotation OR
// Clicking 'X' button on mobile dialog to close
case constants.DATA_TYPE_CANCEL:
if (this.isMobile) {
this.hide();
} else {
// Cancels + destroys the annotation thread
this.cancelAnnotation();
}
case constants.DATA_TYPE_MOBILE_CLOSE:
this.cancelAnnotation();
break;
// Clicking inside reply text area
case constants.DATA_TYPE_REPLY_TEXTAREA:
Expand All @@ -620,10 +592,9 @@ class AnnotationDialog extends EventEmitter {
this.hideDeleteConfirmation(annotationID);
break;
// Clicking 'Delete' button to confirm deletion
case constants.DATA_TYPE_CONFIRM_DELETE: {
case constants.DATA_TYPE_CONFIRM_DELETE:
this.deleteAnnotation(annotationID);
break;
}

default:
break;
Expand Down
1 change: 1 addition & 0 deletions src/AnnotationThread.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ class AnnotationThread extends EventEmitter {
*/
cancelUnsavedAnnotation() {
if (!util.isPending(this.state)) {
this.hideDialog();
return;
}

Expand Down
18 changes: 3 additions & 15 deletions src/Annotator.js
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import EventEmitter from 'events';
import AnnotationService from './AnnotationService';
import * as util from './util';
import { ICON_CLOSE } from './icons/icons';
import './Annotator.scss';
import {
CLASS_HIDDEN,
DATA_TYPE_ANNOTATION_DIALOG,
CLASS_MOBILE_ANNOTATION_DIALOG,
CLASS_ANNOTATION_DIALOG,
CLASS_MOBILE_DIALOG_HEADER,
CLASS_DIALOG_CLOSE,
ID_MOBILE_ANNOTATION_DIALOG,
TYPES,
STATES,
Expand All @@ -18,8 +16,6 @@ import {
CONTROLLER_EVENT
} from './constants';

const MOBILE_DIALOG_CLOSE = 'mobile-dialog-close';

class Annotator extends EventEmitter {
//--------------------------------------------------------------------------
// Typedef
Expand Down Expand Up @@ -273,29 +269,21 @@ class Annotator extends EventEmitter {
* @return {void}
*/
setupMobileDialog() {
// Generate HTML of dialog
this.mobileDialogEl = document.createElement('div');
this.mobileDialogEl = util.generateMobileDialogEl();
this.mobileDialogEl.setAttribute('data-type', DATA_TYPE_ANNOTATION_DIALOG);
this.mobileDialogEl.classList.add(CLASS_MOBILE_ANNOTATION_DIALOG);
this.mobileDialogEl.classList.add(CLASS_ANNOTATION_DIALOG);
this.mobileDialogEl.classList.add(CLASS_HIDDEN);
this.mobileDialogEl.id = ID_MOBILE_ANNOTATION_DIALOG;
this.container.appendChild(this.mobileDialogEl);

const mobileHeader = document.createElement('div');
mobileHeader.classList.add(CLASS_MOBILE_DIALOG_HEADER);
this.mobileDialogEl.appendChild(mobileHeader);

const closeBtn = util.generateBtn([CLASS_DIALOG_CLOSE], MOBILE_DIALOG_CLOSE, ICON_CLOSE, MOBILE_DIALOG_CLOSE);
mobileHeader.appendChild(closeBtn);
}

/**
* Hides and resets the shared mobile dialog.
*
* @return {void}
*/
resetMobileDialog() {
removeThreadFromSharedDialog() {
if (!this.mobileDialogEl || this.mobileDialogEl.classList.contains(CLASS_HIDDEN)) {
return;
}
Expand Down Expand Up @@ -670,7 +658,7 @@ class Annotator extends EventEmitter {
handleControllerEvents(data) {
switch (data.event) {
case CONTROLLER_EVENT.resetMobileDialog:
this.resetMobileDialog();
this.removeThreadFromSharedDialog();
break;
case CONTROLLER_EVENT.toggleMode:
this.toggleAnnotationMode(data.mode);
Expand Down
37 changes: 12 additions & 25 deletions src/__tests__/AnnotationDialog-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ describe('AnnotationDialog', () => {
expect(stubs.hide).to.not.be.called;
});

it('should hide and reset the mobile annotations dialog', () => {
it('should hide the mobile annotations dialog', () => {
dialog.element = document.querySelector(constants.SELECTOR_MOBILE_ANNOTATION_DIALOG);
stubs.hide = sandbox.stub(util, 'hideElement');
stubs.unbind = sandbox.stub(dialog, 'unbindDOMListeners');
Expand All @@ -219,13 +219,6 @@ describe('AnnotationDialog', () => {
expect(stubs.hide).to.be.called;
expect(stubs.unbind).to.be.called;
expect(stubs.cancel).to.be.called;
expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false;
});

it('should remove the animation class', () => {
dialog.element = document.querySelector(constants.SELECTOR_MOBILE_ANNOTATION_DIALOG);
dialog.hideMobileDialog();
expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false;
});

it('should cancel unsaved annotations only if the dialog does not have annotations', () => {
Expand Down Expand Up @@ -733,69 +726,63 @@ describe('AnnotationDialog', () => {
});

it('should post annotation when post annotation button is clicked', () => {
stubs.findClosest.returns(constants.CLASS_ANNOTATION_BUTTON_POST);
stubs.findClosest.returns(constants.DATA_TYPE_POST);

dialog.clickHandler(stubs.event);
expect(stubs.post).to.be.called;
});

it('should cancel annotation when cancel annotation button is clicked', () => {
stubs.findClosest.returns(constants.CLASS_ANNOTATION_BUTTON_CANCEL);
dialog.isMobile = false;

stubs.findClosest.returns(constants.DATA_TYPE_CANCEL);
dialog.clickHandler(stubs.event);
expect(stubs.cancel).to.be.called;
expect(stubs.hideMobile).to.not.be.called;
});

it('should only hide the mobile dialog when the cancel annotation button is clicked on mobile', () => {
stubs.findClosest.returns(constants.CLASS_ANNOTATION_BUTTON_CANCEL);
dialog.isMobile = true;

it('should cancel annotation when mobile dialog close button is clicked', () => {
stubs.findClosest.returns(constants.DATA_TYPE_MOBILE_CLOSE);
dialog.clickHandler(stubs.event);
expect(stubs.cancel).to.not.be.called;
expect(stubs.hideMobile).to.be.called;
expect(stubs.cancel).to.be.called;
});

it('should activate reply area when textarea is clicked', () => {
stubs.findClosest.returns(CLASS_REPLY_TEXTAREA);
stubs.findClosest.returns(constants.DATA_TYPE_REPLY_TEXTAREA);

dialog.clickHandler(stubs.event);
expect(stubs.activate).to.be.called;
});

it('should deactivate reply area when cancel reply button is clicked', () => {
stubs.findClosest.returns('cancel-reply-btn');
stubs.findClosest.returns(constants.DATA_TYPE_CANCEL_REPLY);

dialog.clickHandler(stubs.event);
expect(stubs.deactivate).to.be.calledWith(true);
});

it('should post reply when post reply button is clicked', () => {
stubs.findClosest.returns('post-reply-btn');
stubs.findClosest.returns(constants.DATA_TYPE_POST_REPLY);

dialog.clickHandler(stubs.event);
expect(stubs.reply).to.be.called;
});

it('should show delete confirmation when delete button is clicked', () => {
stubs.findClosest.onFirstCall().returns('delete-btn');
stubs.findClosest.onFirstCall().returns(constants.DATA_TYPE_DELETE);
stubs.findClosest.onSecondCall().returns('someID');

dialog.clickHandler(stubs.event);
expect(stubs.showDelete).to.be.calledWith('someID');
});

it('should cancel deletion when cancel delete button is clicked', () => {
stubs.findClosest.onFirstCall().returns(CLASS_CANCEL_DELETE);
stubs.findClosest.onFirstCall().returns(constants.DATA_TYPE_CANCEL_DELETE);
stubs.findClosest.onSecondCall().returns('someID');

dialog.clickHandler(stubs.event);
expect(stubs.hideDelete).to.be.calledWith('someID');
});

it('should confirm deletion when confirm delete button is clicked', () => {
stubs.findClosest.onFirstCall().returns('confirm-delete-btn');
stubs.findClosest.onFirstCall().returns(constants.DATA_TYPE_CONFIRM_DELETE);
stubs.findClosest.onSecondCall().returns('someID');

dialog.clickHandler(stubs.event);
Expand Down
25 changes: 12 additions & 13 deletions src/__tests__/AnnotationThread-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,27 +743,26 @@ describe('AnnotationThread', () => {
});

describe('cancelUnsavedAnnotation()', () => {
it('should only destroy thread if on a mobile browser or in a pending/pending-active state', () => {
it('should destroy thread if in a pending/pending-active state', () => {
sandbox.stub(thread, 'destroy');
sandbox.stub(thread, 'hideDialog');
stubs.isPending = sandbox.stub(util, 'isPending').returns(true);

// mobile
thread.isMobile = true;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;
expect(thread.emit).to.be.calledWith(THREAD_EVENT.cancel);
expect(thread.hideDialog).to.not.be.called;
});

// 'pending' state
thread.isMobile = false;
thread.state = STATES.pending;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;
expect(thread.emit).to.be.calledWith(THREAD_EVENT.cancel);
it('should not destroy thread if not in a pending/pending-active state', () => {
sandbox.stub(thread, 'destroy');
sandbox.stub(thread, 'hideDialog');
stubs.isPending = sandbox.stub(util, 'isPending').returns(false);

// 'pending-active' state
thread.state = STATES.pending_active;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;
expect(thread.emit).to.be.calledWith(THREAD_EVENT.cancel);
expect(thread.destroy).to.not.be.called;
expect(thread.emit).to.not.be.calledWith(THREAD_EVENT.cancel);
expect(thread.hideDialog).to.be.called;
});
});

Expand Down
12 changes: 6 additions & 6 deletions src/__tests__/Annotator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,14 @@ describe('Annotator', () => {
});
});

describe('resetMobileDialog()', () => {
describe('removeThreadFromSharedDialog()', () => {
beforeEach(() => {
sandbox.stub(util, 'hideElement');
sandbox.stub(util, 'showElement');
});

it('should do nothing if the mobile dialog does not exist or is hidden', () => {
annotator.resetMobileDialog();
annotator.removeThreadFromSharedDialog();
expect(util.hideElement).to.not.be.called;

annotator.mobileDialogEl = {
Expand All @@ -181,15 +181,15 @@ describe('Annotator', () => {
removeChild: sandbox.stub(),
lastChild: {}
};
annotator.resetMobileDialog();
annotator.removeThreadFromSharedDialog();
expect(util.hideElement).to.not.be.called;
});

it('should generate mobile annotations dialog and append to container', () => {
annotator.mobileDialogEl = document.createElement('div');
annotator.mobileDialogEl.appendChild(document.createElement('div'));

annotator.resetMobileDialog();
annotator.removeThreadFromSharedDialog();
expect(util.hideElement).to.be.called;
expect(util.showElement).to.be.called;
expect(annotator.mobileDialogEl.children.length).to.equal(0);
Expand Down Expand Up @@ -526,10 +526,10 @@ describe('Annotator', () => {
});

it('should reset mobile annotation dialog on resetMobileDialog', () => {
sandbox.stub(annotator, 'resetMobileDialog');
sandbox.stub(annotator, 'removeThreadFromSharedDialog');
data.event = CONTROLLER_EVENT.resetMobileDialog;
annotator.handleControllerEvents(data);
expect(annotator.resetMobileDialog).to.be.called;
expect(annotator.removeThreadFromSharedDialog).to.be.called;
});

it('should toggle annotation mode on togglemode', () => {
Expand Down
Loading

0 comments on commit 99ccc22

Please sign in to comment.