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

New: Allowing users to add new mobile point annotations #177

Merged
merged 4 commits into from
Jun 26, 2017
Merged

New: Allowing users to add new mobile point annotations #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e7ff23
New: Allowing users to add new mobile point annotations
pramodsum Jun 19, 2017
bf59594
Chore: Fixing JSDoc in touched files
pramodsum Jun 19, 2017
ff6d9ce
Chore: Clarifying method names
pramodsum Jun 21, 2017
e40c9f5
Chore: Update unit tests
pramodsum Jun 21, 2017
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
17 changes: 14 additions & 3 deletions src/lib/annotations/AnnotationDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog';
dialogCloseButtonEl.removeEventListener('click', this.hideMobileDialog);

annotatorUtil.hideElement(this.element);
this.unbindDOMListeners();

// Cancel any unsaved annotations
if (!this.hasAnnotations) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is cancelAnnotation not safe to call if there are no unsaved annotations?

Copy link
Contributor Author

@pramodsum pramodsum Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's not safe to call it when there are annotations because the method not only cancels the current annotation but calls thread.destroy() if this was the only annotation left in the thread

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that expected behavior? If it's the only potential annotation in the thread and we cancel it then the thread should be destroyed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we call thread.destroy(), is this because we create a temporary annotation, due to the limitations of the "every annotation has its own dialog" system?

this.cancelAnnotation();
}
}

/**
Expand Down Expand Up @@ -218,7 +224,7 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog';
/**
* Sets up the dialog element.
*
* @param {Annotation[]} Annotations - to show in the dialog
* @param {Annotation[]} annotations - to show in the dialog
* @return {void}
* @protected
*/
Expand Down Expand Up @@ -370,7 +376,12 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog';
break;
// Clicking 'Cancel' button to cancel the annotation
case 'cancel-annotation-btn':
this.cancelAnnotation();
if (this.isMobile) {
this.hideMobileDialog();
} else {
this.cancelAnnotation();
}

this.deactivateReply(true);
break;
// Clicking inside reply text area
Expand Down Expand Up @@ -529,7 +540,7 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog';
* Deactivate reply textarea.
*
* @private
* @param {Boolean} clearText - Whether or not text in text area should be cleared
* @param {boolean} clearText - Whether or not text in text area should be cleared
* @return {void}
*/
deactivateReply(clearText) {
Expand Down
29 changes: 25 additions & 4 deletions src/lib/annotations/AnnotationThread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';
* @return {void}
*/
destroy() {
if (this.dialog) {
if (this.dialog && !this.isMobile) {
this.unbindCustomListenersOnDialog();
this.dialog.destroy();
}
Expand Down Expand Up @@ -204,6 +204,10 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';

// If this annotation was the last one in the thread, destroy the thread
} else if (this.annotations.length === 0 || annotatorUtil.isPlainHighlight(this.annotations)) {
if (this.isMobile) {
this.dialog.removeAnnotation(annotationID);
this.dialog.hideMobileDialog();
}
this.destroy();

// Otherwise, remove deleted annotation from dialog
Expand Down Expand Up @@ -353,7 +357,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';
}

this.dialog.addListener('annotationcreate', this.createAnnotation);
this.dialog.addListener('annotationcancel', this.destroy);
this.dialog.addListener('annotationcancel', this.cancelUnsavedAnnotation);
this.dialog.addListener('annotationdelete', this.deleteAnnotationWithID);
}

Expand All @@ -373,6 +377,23 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';
this.dialog.removeAllListeners('annotationdelete');
}

/**
* Destroys mobile and pending/pending-active annotation threads
*
* @protected
* @return {void}
*/
cancelUnsavedAnnotation() {
if (
!this.isMobile &&
this.state !== constants.ANNOTATION_STATE_PENDING &&
this.state !== constants.ANNOTATION_STATE_PENDING_ACTIVE
) {
return;
}
this.destroy();
}

//--------------------------------------------------------------------------
// Private
//--------------------------------------------------------------------------
Expand Down Expand Up @@ -443,7 +464,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';
* Creates a new point annotation
*
* @private
* @param data - Annotation data
* @param {Object} data - Annotation data
* @return {void}
*/
createAnnotation(data) {
Expand All @@ -454,7 +475,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons';
* Deletes annotation with annotationID from thread
*
* @private
* @param data - Annotation data
* @param {Object} data - Annotation data
* @return {void}
*/
deleteAnnotationWithID(data) {
Expand Down
28 changes: 28 additions & 0 deletions src/lib/annotations/__tests__/AnnotationDialog-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,14 @@ describe('lib/annotations/AnnotationDialog', () => {
it('should hide and reset the mobile annotations dialog', () => {
dialog.element = document.querySelector('.bp-mobile-annotation-dialog');
stubs.hide = sandbox.stub(annotatorUtil, 'hideElement');
stubs.unbind = sandbox.stub(dialog, 'unbindDOMListeners');
stubs.cancel = sandbox.stub(dialog, 'cancelAnnotation');
dialog.hasAnnotations = true;

dialog.hideMobileDialog();
expect(stubs.hide).to.be.called;
expect(stubs.unbind).to.be.called;
expect(stubs.cancel).to.not.be.called;
expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false;
});

Expand All @@ -192,6 +198,15 @@ describe('lib/annotations/AnnotationDialog', () => {
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', () => {
dialog.element = document.querySelector('.bp-mobile-annotation-dialog');
stubs.cancel = sandbox.stub(dialog, 'cancelAnnotation');
dialog.hasAnnotations = false;

dialog.hideMobileDialog();
expect(stubs.cancel).to.be.called;
});
});

describe('hide()', () => {
Expand Down Expand Up @@ -460,6 +475,7 @@ describe('lib/annotations/AnnotationDialog', () => {
stubs.hideDelete = sandbox.stub(dialog, 'hideDeleteConfirmation');
stubs.delete = sandbox.stub(dialog, 'deleteAnnotation');
stubs.reply = sandbox.stub(dialog, 'postReply');
stubs.hideMobile = sandbox.stub(dialog, 'hideMobileDialog');
});

it('should post annotation when post annotation button is clicked', () => {
Expand All @@ -471,9 +487,21 @@ describe('lib/annotations/AnnotationDialog', () => {

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

dialog.clickHandler(stubs.event);
expect(stubs.cancel).to.be.called;
expect(stubs.hideMobile).to.not.be.called;
expect(stubs.deactivate).to.be.calledWith(true);
});

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

dialog.clickHandler(stubs.event);
expect(stubs.cancel).to.not.be.called;
expect(stubs.hideMobile).to.be.called;
expect(stubs.deactivate).to.be.calledWith(true);
});

Expand Down
46 changes: 45 additions & 1 deletion src/lib/annotations/__tests__/AnnotationThread-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,17 @@ describe('lib/annotations/AnnotationThread', () => {
expect(stubs.unbindDOM).to.be.called;
expect(stubs.emit).to.be.calledWith('threaddeleted');
});

it('should not destroy the dialog on mobile', () => {
stubs.unbindCustom = sandbox.stub(thread, 'unbindCustomListenersOnDialog');
stubs.destroyDialog = sandbox.stub(thread.dialog, 'destroy');
thread.element = null;
thread.isMobile = true;

thread.destroy();
expect(stubs.unbindCustom).to.not.be.called;
expect(stubs.destroyDialog).to.not.be.called;
});
});

describe('hide()', () => {
Expand Down Expand Up @@ -199,7 +210,8 @@ describe('lib/annotations/AnnotationThread', () => {
removeAllListeners: () => {},
show: () => {},
hide: () => {},
removeAnnotation: () => {}
removeAnnotation: () => {},
hideMobileDialog: () => {}
};
stubs.dialogMock = sandbox.mock(thread.dialog);

Expand All @@ -210,10 +222,20 @@ describe('lib/annotations/AnnotationThread', () => {
});

it('should destroy the thread if the deleted annotation was the last annotation in the thread', () => {
thread.isMobile = false;
stubs.dialogMock.expects('removeAnnotation').never();
stubs.dialogMock.expects('hideMobileDialog').never();
thread.deleteAnnotation('someID', false);
expect(stubs.destroy).to.be.called;
});

it('should destroy the thread and hide the mobile dialog if the deleted annotation was the last annotation in the thread on mobile', () => {
thread.isMobile = true;
stubs.dialogMock.expects('removeAnnotation');
stubs.dialogMock.expects('hideMobileDialog');
thread.deleteAnnotation('someID', false);
});

it('should remove the relevant annotation from its dialog if the deleted annotation was not the last one', () => {
// Add another annotation to thread so 'someID' isn't the only annotation
thread.annotations.push(stubs.annotation2);
Expand Down Expand Up @@ -437,6 +459,28 @@ describe('lib/annotations/AnnotationThread', () => {
});
});

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

// mobile
thread.isMobile = true;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;

// 'pending' state
thread.isMobile = false;
thread.state = constants.ANNOTATION_STATE_PENDING;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;

// 'pending-active' state
thread.state = constants.ANNOTATION_STATE_PENDING_ACTIVE;
thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;
});
});

describe('createElement()', () => {
it('should create an element with the right class and attribute', () => {
const element = thread.createElement();
Expand Down
5 changes: 2 additions & 3 deletions src/lib/viewers/BaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300;
/**
* [constructor]
*
* @param {HTMLElement} containerEl - The container
* @param {Object} options - some options
* @return {BaseViewer} Instance of base viewer
*/
Expand Down Expand Up @@ -583,7 +582,7 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300;
const { file } = this.options;

// Users can currently only view annotations on mobile
this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE) && !this.isMobile;
this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE);
if (this.canAnnotate) {
this.showAnnotateButton(this.getPointModeClickHandler());
}
Expand All @@ -592,7 +591,7 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300;
}

/**
* Orient anntations to the correct scale and orientatio of the annotated document.
* Orient annotations to the correct scale and orientation of the annotated document.
* @TODO(jholdstock|spramod): Remove this once we are emitting the correct messaging.
*
* @protected
Expand Down