From d90e72c22bb298231738c9be4604309de4db963e Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 14 Nov 2017 08:47:00 -0800 Subject: [PATCH] Fix: Validation message fails to get displayed when user clicks on 'Post' without entering any comment while adding a Point Annotation. (#27) * Chore: Indicate invalid input on annotation post * Update: Add validation to CommentBox.js as well * Update: consolidating focus methods --- src/AnnotationDialog.js | 40 +++++++++++++++++++++++ src/Annotator.scss | 4 +++ src/CommentBox.js | 28 +++++++++++++--- src/__tests__/AnnotationDialog-test.js | 44 +++++++++++++++++++++++++- src/__tests__/CommentBox-test.js | 44 +++++++++++++++++++++++--- src/annotationConstants.js | 1 + 6 files changed, 151 insertions(+), 10 deletions(-) diff --git a/src/AnnotationDialog.js b/src/AnnotationDialog.js index 8141a9c5f..b72f93d29 100644 --- a/src/AnnotationDialog.js +++ b/src/AnnotationDialog.js @@ -55,6 +55,8 @@ class AnnotationDialog extends EventEmitter { this.canAnnotate = data.canAnnotate; this.locale = data.locale; this.isMobile = data.isMobile; + + this.validateTextArea = this.validateTextArea.bind(this); } /** @@ -240,6 +242,7 @@ class AnnotationDialog extends EventEmitter { const annotationTextEl = this.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); const text = textInput || annotationTextEl.value; if (text.trim() === '') { + annotationTextEl.classList.add(constants.CLASS_INVALID_INPUT); return; } @@ -330,12 +333,38 @@ class AnnotationDialog extends EventEmitter { this.element.addEventListener('mouseup', this.stopPropagation); this.element.addEventListener('wheel', this.stopPropagation); + const replyTextEl = this.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); + if (replyTextEl) { + replyTextEl.addEventListener('focus', this.validateTextArea); + } + + const annotationTextEl = this.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); + if (annotationTextEl) { + annotationTextEl.addEventListener('focus', this.validateTextArea); + } + if (!this.isMobile) { this.element.addEventListener('mouseenter', this.mouseenterHandler); this.element.addEventListener('mouseleave', this.mouseleaveHandler); } } + /** + * Removes red border around textarea on focus + * + * @protected + * @param {Event} event Keyboard event + * @return {void} + */ + validateTextArea(event) { + const textEl = event.target; + if (textEl.type !== 'textarea' || textEl.value.trim() === '') { + return; + } + + textEl.classList.remove(constants.CLASS_INVALID_INPUT); + } + /** * Unbinds DOM event listeners. * @@ -348,6 +377,16 @@ class AnnotationDialog extends EventEmitter { this.element.removeEventListener('mouseup', this.stopPropagation); this.element.removeEventListener('wheel', this.stopPropagation); + const replyTextEl = this.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); + if (replyTextEl) { + replyTextEl.removeEventListener('focus', this.validateTextArea); + } + + const annotationTextEl = this.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); + if (annotationTextEl) { + annotationTextEl.removeEventListener('focus', this.validateTextArea); + } + if (!this.isMobile) { this.element.removeEventListener('mouseenter', this.mouseenterHandler); this.element.removeEventListener('mouseleave', this.mouseleaveHandler); @@ -626,6 +665,7 @@ class AnnotationDialog extends EventEmitter { const replyTextEl = this.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); const text = replyTextEl.value; if (text.trim() === '') { + replyTextEl.classList.add(constants.CLASS_INVALID_INPUT); return; } diff --git a/src/Annotator.scss b/src/Annotator.scss index 68d977771..ce008c525 100644 --- a/src/Annotator.scss +++ b/src/Annotator.scss @@ -204,6 +204,10 @@ $tablet: 'max-width: 768px'; &.bp-is-active { min-height: 68px; } + + &.bp-invalid-input { + border-color: fade-out($great-balls-of-fire, .5); + } } .annotation-comment { diff --git a/src/CommentBox.js b/src/CommentBox.js index 4318c28cb..08cd0f6b4 100644 --- a/src/CommentBox.js +++ b/src/CommentBox.js @@ -86,6 +86,7 @@ class CommentBox extends EventEmitter { this.placeholderText = config.localized.addCommentPlaceholder; // Explicit scope binding for event listeners + this.focus = this.focus.bind(this); this.onCancel = this.onCancel.bind(this); this.onPost = this.onPost.bind(this); } @@ -98,6 +99,7 @@ class CommentBox extends EventEmitter { focus() { if (this.textAreaEl) { this.textAreaEl.focus(); + this.textAreaEl.classList.remove(constants.CLASS_INVALID_INPUT); } } @@ -161,12 +163,21 @@ class CommentBox extends EventEmitter { this.containerEl.remove(); this.parentEl = null; this.containerEl = null; - this.cancelEl.removeEventListener('click', this.onCancel); - this.postEl.removeEventListener('click', this.onPost); - if (this.hasTouch) { + + if (this.cancelEl) { + this.cancelEl.removeEventListener('click', this.onCancel); this.cancelEl.removeEventListener('touchstart', this.onCancel); + } + + if (this.postEl) { + this.postEl.removeEventListener('click', this.onPost); this.postEl.removeEventListener('touchstart', this.onPost); } + + if (this.textAreaEl) { + this.textAreaEl.removeEventListener('focus', this.focus); + this.textAreaEl.removeEventListener('keydown', this.focus); + } } //-------------------------------------------------------------------------- @@ -232,7 +243,14 @@ class CommentBox extends EventEmitter { onPost(event) { // stops touch propogating to a click event this.preventDefaultAndPropagation(event); - this.emit(CommentBox.CommentEvents.post, this.textAreaEl.value); + + const text = this.textAreaEl.value; + if (text.trim() === '') { + this.textAreaEl.classList.add(constants.CLASS_INVALID_INPUT); + return; + } + + this.emit(CommentBox.CommentEvents.post, text); this.clear(); } @@ -252,9 +270,11 @@ class CommentBox extends EventEmitter { this.postEl = containerEl.querySelector(constants.SELECTOR_ANNOTATION_BUTTON_POST); // Add event listeners + this.textAreaEl.addEventListener('focus', this.focus); this.cancelEl.addEventListener('click', this.onCancel); this.postEl.addEventListener('click', this.onPost); if (this.hasTouch) { + this.textAreaEl.addEventListener('keydown', this.focus); containerEl.addEventListener('touchend', this.preventDefaultAndPropagation.bind(this)); this.cancelEl.addEventListener('touchend', this.onCancel); this.postEl.addEventListener('touchend', this.onPost); diff --git a/src/__tests__/AnnotationDialog-test.js b/src/__tests__/AnnotationDialog-test.js index dacf2acf9..e8533c3fc 100644 --- a/src/__tests__/AnnotationDialog-test.js +++ b/src/__tests__/AnnotationDialog-test.js @@ -13,6 +13,7 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog'; const CLASS_BUTTON_DELETE_COMMENT = 'delete-comment-btn'; const CLASS_COMMENTS_CONTAINER = 'annotation-comments'; const SELECTOR_DELETE_CONFIRMATION = '.delete-confirmation'; +const CLASS_INVALID_INPUT = 'bp-invalid-input'; let dialog; const sandbox = sinon.sandbox.create(); @@ -395,6 +396,10 @@ describe('AnnotationDialog', () => { describe('bindDOMListeners()', () => { it('should bind DOM listeners', () => { stubs.add = sandbox.stub(dialog.element, 'addEventListener'); + const replyTextEl = dialog.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); + sandbox.stub(replyTextEl, 'addEventListener'); + const annotationTextEl = dialog.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); + sandbox.stub(annotationTextEl, 'addEventListener'); dialog.bindDOMListeners(); expect(stubs.add).to.be.calledWith('keydown', sinon.match.func); @@ -403,6 +408,8 @@ describe('AnnotationDialog', () => { expect(stubs.add).to.be.calledWith('mouseenter', sinon.match.func); expect(stubs.add).to.be.calledWith('mouseleave', sinon.match.func); expect(stubs.add).to.be.calledWith('wheel', sinon.match.func); + expect(replyTextEl.addEventListener).to.be.calledWith('focus', sinon.match.func); + expect(annotationTextEl.addEventListener).to.be.calledWith('focus', sinon.match.func); }); it('should not bind mouseenter/leave events for mobile browsers', () => { @@ -416,12 +423,41 @@ describe('AnnotationDialog', () => { expect(stubs.add).to.not.be.calledWith('mouseenter', sinon.match.func); expect(stubs.add).to.not.be.calledWith('mouseleave', sinon.match.func); expect(stubs.add).to.be.calledWith('wheel', sinon.match.func); + dialog.element = null; + }); + }); + + describe('validateTextArea()', () => { + it('should do nothing if keyboard event was not in a textarea', () => { + stubs.textEl = document.createElement('div'); + stubs.textEl.classList.add(constants.CLASS_INVALID_INPUT); + dialog.validateTextArea({ target: stubs.textEl }); + expect(stubs.textEl).to.have.class(constants.CLASS_INVALID_INPUT); + }); + + it('should do nothing if textarea is blank', () => { + stubs.textEl = document.createElement('textarea'); + stubs.textEl.classList.add(constants.CLASS_INVALID_INPUT); + dialog.validateTextArea({ target: stubs.textEl }); + expect(stubs.textEl).to.have.class(constants.CLASS_INVALID_INPUT); + }); + + it('should remove red border around textarea', () => { + stubs.textEl = document.createElement('textarea'); + stubs.textEl.classList.add(constants.CLASS_INVALID_INPUT); + stubs.textEl.value = 'words'; + dialog.validateTextArea({ target: stubs.textEl }); + expect(stubs.textEl).to.not.have.class(constants.CLASS_INVALID_INPUT); }); }); describe('unbindDOMListeners()', () => { it('should unbind DOM listeners', () => { stubs.remove = sandbox.stub(dialog.element, 'removeEventListener'); + const replyTextEl = dialog.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); + sandbox.stub(replyTextEl, 'removeEventListener'); + const annotationTextEl = dialog.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); + sandbox.stub(annotationTextEl, 'removeEventListener'); dialog.unbindDOMListeners(); expect(stubs.remove).to.be.calledWith('keydown', sinon.match.func); @@ -430,6 +466,8 @@ describe('AnnotationDialog', () => { expect(stubs.remove).to.be.calledWith('mouseenter', sinon.match.func); expect(stubs.remove).to.be.calledWith('mouseleave', sinon.match.func); expect(stubs.remove).to.be.calledWith('wheel', sinon.match.func); + expect(replyTextEl.removeEventListener).to.be.calledWith('focus', dialog.validateTextArea); + expect(annotationTextEl.removeEventListener).to.be.calledWith('focus', dialog.validateTextArea); }); it('should not bind mouseenter/leave events for mobile browsers', () => { @@ -750,10 +788,12 @@ describe('AnnotationDialog', () => { }); }); - describe('_postAannotation()', () => { + describe('postAnnotation()', () => { it('should not post an annotation to the dialog if it has no text', () => { + const annotationTextEl = dialog.element.querySelector(constants.SELECTOR_ANNOTATION_TEXTAREA); dialog.postAnnotation(); expect(stubs.emit).to.not.be.called; + expect(annotationTextEl).to.have.class(CLASS_INVALID_INPUT); }); it('should post an annotation to the dialog if it has text', () => { @@ -851,9 +891,11 @@ describe('AnnotationDialog', () => { }) ); dialog.activateReply(); + const replyTextEl = dialog.element.querySelector(`.${CLASS_REPLY_TEXTAREA}`); dialog.postReply(); expect(stubs.emit).to.not.be.called; + expect(replyTextEl).to.have.class(CLASS_INVALID_INPUT); }); it('should post a reply to the dialog if it has text', () => { diff --git a/src/__tests__/CommentBox-test.js b/src/__tests__/CommentBox-test.js index af6de4010..de50694f1 100644 --- a/src/__tests__/CommentBox-test.js +++ b/src/__tests__/CommentBox-test.js @@ -3,7 +3,8 @@ import CommentBox from '../CommentBox'; import { CLASS_HIDDEN, SELECTOR_ANNOTATION_BUTTON_CANCEL, - SELECTOR_ANNOTATION_BUTTON_POST + SELECTOR_ANNOTATION_BUTTON_POST, + CLASS_INVALID_INPUT } from '../annotationConstants'; describe('CommentBox', () => { @@ -172,6 +173,13 @@ describe('CommentBox', () => { commentBox.destroy(); expect(remove).to.be.called; }); + + it('should remove event listener from text area', () => { + commentBox.show(); + const remove = sandbox.stub(commentBox.textAreaEl, 'removeEventListener'); + commentBox.destroy(); + expect(remove).to.be.called; + }); }); describe('createHTML()', () => { @@ -218,15 +226,22 @@ describe('CommentBox', () => { describe('onPost()', () => { beforeEach(() => { + commentBox.textAreaEl = document.createElement('textarea'); sandbox.stub(commentBox, 'preventDefaultAndPropagation'); }); + it('should invalidate textarea and do nothing if textarea is blank', () => { + const emit = sandbox.stub(commentBox, 'emit'); + const text = ''; + commentBox.onPost({ preventDefault: () => {} }); + expect(emit).to.not.be.calledWith(CommentBox.CommentEvents.post, text); + expect(commentBox.textAreaEl).to.have.class(CLASS_INVALID_INPUT); + }); + it('should emit a post event with the value of the text box', () => { const emit = sandbox.stub(commentBox, 'emit'); const text = 'a comment'; - commentBox.textAreaEl = { - value: text - }; + commentBox.textAreaEl.value = text; commentBox.onPost({ preventDefault: () => {} }); expect(emit).to.be.calledWith(CommentBox.CommentEvents.post, text); }); @@ -259,17 +274,36 @@ describe('CommentBox', () => { expect(commentBox.postEl).to.exist; }); - it('should add an event listener on the cancel and post buttons', () => { + it('should add an event listener on the textarea, cancel and post buttons', () => { + const uiElement = { + addEventListener: sandbox.stub() + }; + const el = document.createElement('section'); + sandbox.stub(el, 'querySelector').returns(uiElement); + sandbox.stub(commentBox, 'createHTML').returns(el); + + commentBox.createCommentBox(); + expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onCancel); + expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onPost); + expect(uiElement.addEventListener).to.be.calledWith('focus', commentBox.focus); + }); + + it('should add an event listener on the textarea, cancel and post buttons if the user is on a touch-enabled mobile device', () => { const uiElement = { addEventListener: sandbox.stub() }; const el = document.createElement('section'); sandbox.stub(el, 'querySelector').returns(uiElement); sandbox.stub(commentBox, 'createHTML').returns(el); + commentBox.hasTouch = true; commentBox.createCommentBox(); expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onCancel); expect(uiElement.addEventListener).to.be.calledWith('click', commentBox.onPost); + expect(uiElement.addEventListener).to.be.calledWith('focus', commentBox.focus); + expect(uiElement.addEventListener).to.be.calledWith('keydown', commentBox.focus); + expect(uiElement.addEventListener).to.be.calledWith('touchend', commentBox.onCancel); + expect(uiElement.addEventListener).to.be.calledWith('touchend', commentBox.onPost); }); }); }); diff --git a/src/annotationConstants.js b/src/annotationConstants.js index c2c2ff8dd..4a6163d11 100644 --- a/src/annotationConstants.js +++ b/src/annotationConstants.js @@ -45,6 +45,7 @@ export const CLASS_ANNOTATION_DRAWING_HEADER = 'bp-annotate-draw-header'; export const CLASS_ANNNOTATION_DRAWING_BACKGROUND = 'bp-annotate-draw-background'; export const CLASS_ADD_DRAWING_BTN = 'bp-btn-annotate-draw-add'; export const CLASS_DELETE_DRAWING_BTN = 'bp-btn-annotate-draw-delete'; +export const CLASS_INVALID_INPUT = 'bp-invalid-input'; export const DATA_TYPE_ANNOTATION_DIALOG = 'annotation-dialog'; export const DATA_TYPE_ANNOTATION_INDICATOR = 'annotation-indicator';