Fix: Merge conflicts + PR comments

box · Nov 14, 2017 · 0e7ed8e · 0e7ed8e
1 parent aef55ba
commit 0e7ed8e
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 40 deletions.
diff --git a/src/CommentBox.js b/src/CommentBox.js
@@ -87,12 +87,6 @@ class CommentBox extends EventEmitter {
         this.placeholderText = config.localized.addCommentPlaceholder;
 
         this.containerEl = this.createCommentBox();
-
-        // Explicit scope binding for event listeners
-        this.focus = this.focus.bind(this);
-        this.onCancel = this.onCancel.bind(this);
-        this.onPost = this.onPost.bind(this);
-        this.preventDefaultAndPropagation = this.preventDefaultAndPropagation.bind(this);
     }
 
     /**
@@ -164,23 +158,24 @@ class CommentBox extends EventEmitter {
             return;
         }
 
+        this.containerEl.removeEventListener('touchend', this.preventDefaultAndPropagation);
+
         this.containerEl.remove();
         this.parentEl = null;
         this.containerEl = null;
 
         if (this.cancelEl) {
             this.cancelEl.removeEventListener('click', this.onCancel);
-            this.cancelEl.removeEventListener('touchstart', this.onCancel);
+            this.cancelEl.removeEventListener('touchend', this.onCancel);
         }
 
         if (this.postEl) {
             this.postEl.removeEventListener('click', this.onPost);
-            this.postEl.removeEventListener('touchstart', this.onPost);
+            this.postEl.removeEventListener('touchend', this.onPost);
         }
 
         if (this.textAreaEl) {
             this.textAreaEl.removeEventListener('focus', this.focus);
-            this.textAreaEl.removeEventListener('keydown', this.focus);
         }
     }
 
@@ -274,15 +269,16 @@ 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);
-            this.cancelEl.addEventListener('touchend', this.onCancel);
-            this.postEl.addEventListener('touchend', this.onPost);
+            this.textAreaEl.addEventListener('focus', this.focus.bind(this));
+            containerEl.addEventListener('touchend', this.preventDefaultAndPropagation.bind(this));
+            this.cancelEl.addEventListener('touchend', this.onCancel.bind(this));
+            this.postEl.addEventListener('touchend', this.onPost.bind(this));
+        } else {
+            this.textAreaEl.addEventListener('focus', this.focus);
+            this.cancelEl.addEventListener('click', this.onCancel);
+            this.postEl.addEventListener('click', this.onPost);
         }
 
         return containerEl;

diff --git a/src/__tests__/CommentBox-test.js b/src/__tests__/CommentBox-test.js
@@ -275,7 +275,8 @@ describe('CommentBox', () => {
 
         it('should add an event listener on the textarea, cancel and post buttons', () => {
             const uiElement = {
-                addEventListener: sandbox.stub()
+                addEventListener: sandbox.stub(),
+                removeEventListener: sandbox.stub()
             };
             const el = document.createElement('section');
             sandbox.stub(el, 'querySelector').returns(uiElement);
@@ -289,20 +290,19 @@ describe('CommentBox', () => {
 
         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()
+                addEventListener: sandbox.stub(),
+                removeEventListener: 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);
+            expect(uiElement.addEventListener).to.be.calledWith('focus', sinon.match.func);
+            expect(uiElement.addEventListener).to.be.calledWith('touchend', sinon.match.func);
+            expect(uiElement.addEventListener).to.be.calledWith('touchend', sinon.match.func);
+            expect(uiElement.addEventListener).to.be.calledWith('touchend', sinon.match.func);
 
             commentBox.containerEl = null;
         });

diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js
@@ -23,28 +23,46 @@ class PointModeController extends AnnotationModeController {
             hasTouch: options.hasTouch,
             localized: options.localized
         });
+        this.createDialog.createElement();
 
-        this.createDialog.addListener(CREATE_EVENT.init, () => this.emit(THREAD_EVENT.pending, TYPES.point));
+        this.onDialogCancel = this.onDialogCancel.bind(this);
+        this.onDialogPost = this.onDialogPost.bind(this);
+        this.destroyPendingThreads = this.destroyPendingThreads.bind(this);
 
-        this.createDialog.addListener(CREATE_EVENT.cancel, () => {
-            const thread = this.getThreadByID(this.pendingThreadID);
-            this.unregisterThread(thread);
-            thread.destroy();
+        this.createDialog.addListener(CREATE_EVENT.init, () => this.emit(THREAD_EVENT.pending, TYPES.point));
+        this.createDialog.addListener(CREATE_EVENT.cancel, this.onDialogCancel);
+        this.createDialog.addListener(CREATE_EVENT.post, this.onDialogPost);
+    }
 
-            this.hideSharedDialog();
-        });
+    /**
+     * Unregister/destroy the pending thread and then clear the create dialog
+     *
+     * @private
+     * @return {void}
+     */
+    onDialogCancel() {
+        const thread = this.getThreadByID(this.pendingThreadID);
+        this.unregisterThread(thread);
+        thread.destroy();
 
-        this.createDialog.addListener(CREATE_EVENT.post, (commentText) => {
-            this.emit(CONTROLLER_EVENT.createThread, {
-                commentText,
-                lastPointEvent: this.lastPointEvent,
-                pendingThreadID: this.pendingThreadID
-            });
+        this.hideSharedDialog();
+    }
 
-            this.hideSharedDialog();
+    /**
+     * Notify listeners of post event and then clear the create dialog
+     *
+     * @private
+     * @param {string} commentText Annotation comment text
+     * @return {void}
+     */
+    onDialogPost(commentText) {
+        this.emit(CONTROLLER_EVENT.createThread, {
+            commentText,
+            lastPointEvent: this.lastPointEvent,
+            pendingThreadID: this.pendingThreadID
         });
 
-        this.destroyPendingThreads = this.destroyPendingThreads.bind(this);
+        this.hideSharedDialog();
     }
 
     /**

diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js
@@ -22,7 +22,8 @@ describe('controllers/PointModeController', () => {
         controller = new PointModeController();
         stubs.thread = {
             show: () => {},
-            getThreadEventData: () => {}
+            getThreadEventData: () => {},
+            destroy: () => {}
         };
         stubs.threadMock = sandbox.mock(stubs.thread);
 
@@ -54,6 +55,35 @@ describe('controllers/PointModeController', () => {
         });
     });
 
+    describe('onDialogCancel()', () => {
+        it('should unregister/destroy the pending thread and clear the create dialog', () => {
+            sandbox.stub(controller, 'getThreadByID').returns(stubs.thread);
+            sandbox.stub(controller, 'unregisterThread');
+            sandbox.stub(controller, 'hideSharedDialog');
+
+            stubs.threadMock.expects('destroy');
+            controller.onDialogCancel();
+            expect(controller.unregisterThread).to.be.calledWith(stubs.thread);
+            expect(controller.hideSharedDialog).to.be.called;
+        });
+    });
+
+    describe('onDialogPost()', () => {
+        it('should notify listeners of post event and clear the create dialog', () => {
+            sandbox.stub(controller, 'hideSharedDialog');
+            controller.lastPointEvent = {};
+            controller.pendingThreadID = '123abc';
+
+            controller.onDialogPost('text');
+            expect(controller.emit).to.be.calledWith(CONTROLLER_EVENT.createThread, {
+                commentText: 'text',
+                lastPointEvent: {},
+                pendingThreadID: '123abc'
+            });
+            expect(controller.hideSharedDialog).to.be.called;
+        });
+    });
+
     describe('hideSharedDialog', () => {
         it('should not hide the shared annotation dialog if already hidden', () => {
             controller.createDialog = { hide: () => {} };