Breaking: Remove hover behavior to display dialogs #155

pramodsum · 2018-03-28T23:13:32Z

Remove min-height animation when opening dialog
Remove hover to open/close dialogs, dialogs open when indicator is clicked and close when you click outside

Based off of #137

boxcla · 2018-03-28T23:13:33Z

Verified that @pramodsum has signed the CLA. Thanks for the pull request!

- Remove min-height animation when opening dialog - Remove hover to open/close dialogs, dialogs open when indicator is clicked and close when you click outside

jeremypress

Great start! This will remove a ton of tech debt

jeremypress · 2018-04-19T16:59:50Z

src/AnnotationDialog.js

@@ -59,11 +59,6 @@ class AnnotationDialog extends EventEmitter {
        this.clickHandler = this.clickHandler.bind(this);
        this.stopPropagation = this.stopPropagation.bind(this);
        this.validateTextArea = this.validateTextArea.bind(this);
-
-        if (!this.isMobile) {


jeremypress · 2018-04-19T17:00:34Z

src/AnnotationDialog.js

@@ -356,6 +351,11 @@ class AnnotationDialog extends EventEmitter {
        this.element.addEventListener('wheel', this.stopPropagation);
        this.element.addEventListener('mouseup', this.stopPropagation);

+        if (!this.isMobile) {


Why is this being bound earlier now?

no particular reason. I think it was just shuffled around

jeremypress · 2018-04-19T17:02:41Z

src/AnnotationDialog.js

                break;
            // Clicking inside reply text area
            case constants.DATA_TYPE_REPLY_TEXTAREA:
                this.activateReply();
                break;
            // Canceling a reply
            case constants.DATA_TYPE_CANCEL_REPLY:
-                this.deactivateReply(true);
+                // this.deactivateReply(true);


Remove this line

jeremypress · 2018-04-19T17:03:05Z

src/AnnotationDialog.js

                break;
            // Clicking inside reply text area
            case constants.DATA_TYPE_REPLY_TEXTAREA:
                this.activateReply();
                break;
            // Canceling a reply
            case constants.DATA_TYPE_CANCEL_REPLY:
-                this.deactivateReply(true);


we weren't hiding the dialog before on comment cancel, should we now?

Nope. Undoing this

jeremypress · 2018-04-19T17:27:51Z

src/AnnotationThread.js

@@ -132,7 +132,11 @@ class AnnotationThread extends EventEmitter {
     * @return {void}
     */
    showDialog() {
-        // Prevents the annotations dialog from being created each mousemove
+        if (this.state !== STATES.pending && util.isInAnnotationMode(this.container, TYPES.point)) {


Why can't we allow showing a dialog in point mode?

It's more for consistency w/ drawing annotations. I'm on board w/ allowing users to select existing drawings while in drawing mode if that's a behavior we want to add.

jeremypress · 2018-04-19T17:28:14Z

src/AnnotationThread.js

+            return;
+        }
+
+        // Prevents the annotations dialog from being every call


can you clarify this comment a bit?

jeremypress · 2018-04-19T17:29:18Z

src/controllers/DrawingModeController.js

@@ -177,6 +178,12 @@ class DrawingModeController extends AnnotationModeController {
        thread.handleStart(location);
    }

+    /** @inheritdoc */
+    exit() {
+        this.annotatedElement.classList.remove(CLASS_ANNOTATION_DRAW_MODE);


What are the mode CSS classes used for?

Functional tests mostly

jeremypress · 2018-04-19T17:31:29Z

src/doc/DocAnnotator.js

        }

+        // Hide open threads when clicking on document
+        this.annotatedElement.addEventListener(


make this click if possible

jeremypress · 2018-04-19T17:32:16Z

src/doc/DocDrawingDialog.js

@@ -164,6 +164,10 @@ class DocDrawingDialog extends AnnotationDialog {
            this.pageEl = this.annotatedElement.querySelector(`[data-page-number="${this.location.page}"]`);
        }

+        if (!this.element) {
+            this.setup([]);


Kinda weird that we have to pass in an empty array here

- Point mode: All point annotations are triggerable - Drawing mode: No existing dialogs are triggerable - When not in a mode: all existing annotation dialogs are triggerable

JustinHoldstock · 2018-05-15T20:31:43Z

src/controllers/DrawingModeController.js

+        this.pushElementHandler(
+            this.annotatedElement,
+            'click',
+            (event) => {


Can you extract this function?

Did it for all the drawing handlers while i was at it

jeremypress · 2018-05-15T20:32:20Z

src/AnnotationDialog.js

@@ -373,8 +370,7 @@ class AnnotationDialog extends EventEmitter {

        if (!this.isMobile) {
            this.element.addEventListener('click', this.clickHandler);
-            this.element.addEventListener('mouseenter', this.mouseenterHandler);
-            this.element.addEventListener('mouseleave', this.mouseleaveHandler);
+            this.element.addEventListener('click', this.stopPropagation);


Why is this needed? And why not just add stopPropagation() to the clickHandler?

Turns out it's already there in clickHandler

jeremypress · 2018-05-15T20:38:43Z

src/AnnotationThread.js

@@ -428,6 +420,8 @@ class AnnotationThread extends EventEmitter {
        this.dialog.removeAllListeners('annotationcreate');
        this.dialog.removeAllListeners('annotationcancel');
        this.dialog.removeAllListeners('annotationdelete');
+        this.dialog.removeAllListeners('annotationshow');


Either switch to removeListener or pass in an array https://nodejs.org/api/events.html#events_emitter_removealllisteners_eventname

jeremypress · 2018-05-15T20:39:21Z

src/Annotator.js

@@ -296,6 +297,27 @@ class Annotator extends EventEmitter {
        this.mobileDialogEl.removeChild(this.mobileDialogEl.lastChild);
    }

+    /**
+     * Hides all non-pending annotations if mouse event occurs outside an


What happens if you have a pending annotation open and you click on a different point icon?

it'll close the pending annotation and open the clicked on one

does that data get saved in the pending annotation so you can come back to it?

jeremypress · 2018-05-15T20:40:29Z

src/AnnotationDialog.js

@@ -213,6 +209,7 @@ class AnnotationDialog extends EventEmitter {

        util.hideElement(this.element);
        this.deactivateReply();
+        this.emit('annotationhide');


Not a requirement to merge but you do have most of these as constants elsewhere.

yea i think I never moved the constants that were are emitted from AnnotationDialog.js. I can clean it up later

jeremypress · 2018-05-15T20:41:30Z

src/controllers/AnnotationModeController.js

@@ -330,6 +333,12 @@ class AnnotationModeController extends EventEmitter {
                this.hadPendingThreads = false;
                this.emit(data.event, data.data);
                break;
+            case THREAD_EVENT.show:
+                this.visibleThreadID = data.data.threadID;


unpack data and event at the top for readability?

jeremypress · 2018-05-15T20:41:57Z

src/controllers/AnnotationModeController.js

@@ -362,17 +371,19 @@ class AnnotationModeController extends EventEmitter {
     * @param {HTMLElement} element - The element to bind the listener to
     * @param {Array|string} type - An array of event types to listen for or the event name to listen for
     * @param {Function} handlerFn - The callback to be invoked when the element emits a specified eventname
+     * @param {boolean} useCapture - Whether or not to prioritize handler call


Does this prioritize or block the event from reaching other places?

jeremypress · 2018-05-15T20:43:36Z

src/controllers/DrawingModeController.js

+            },
+            true
+        );
+        this.pushElementHandler(this.annotatedElement, ['mousedown', 'touchstart'], this.drawingStartHandler, true);


This is pretty confusing, can you add a comment why we need to push two element handlers? and why is click separated from mousedown and touchstart?

both are needed for the surface edge cases and click is separate because it more or less just prevents any click events other than drawing ones from ever triggering on the document specifically

jeremypress · 2018-05-15T20:47:06Z

src/doc/DocAnnotator.js

@@ -678,142 +659,6 @@ class DocAnnotator extends Annotator {
        }
    }

-    /**


as Justin would say, "10/10"

jeremypress · 2018-05-15T20:49:11Z

src/util.js

+ * @param {Event} event Mouse event
+ * @return {boolean} Whether or not mouse is inside dialog
+ */
+export function isInAnnotation(event) {


I would rename this because it applies to either being in an annotation or clicking on an annotation icon.

jeremypress · 2018-05-15T21:34:28Z

src/controllers/DrawingModeController.js

+        this.undoDrawing = this.undoDrawing.bind(this);
+        this.redoDrawing = this.redoDrawing.bind(this);
+
+        this.pushElementHandler(this.annotatedElement, 'click', this.stopPropagation, true);


why not stop propagation to drawingStart?

pramodsum changed the title ~~Breaking: Remove hover behavior for Annotations~~ Breaking: Remove hover behavior to display annotation dialogs Mar 29, 2018

pramodsum changed the title ~~Breaking: Remove hover behavior to display annotation dialogs~~ Breaking: Remove hover behavior to display dialogs Mar 29, 2018

Breaking: Prototype for simplifying annotations

5944cd2

- Remove min-height animation when opening dialog - Remove hover to open/close dialogs, dialogs open when indicator is clicked and close when you click outside

pramodsum force-pushed the hover branch from 1dc5078 to 5944cd2 Compare March 30, 2018 19:28

pramodsum added 5 commits April 6, 2018 11:21

Merge branch 'master' into hover

e93656c

Merge branch 'master' into hover

c5de252

Merge branch 'master' into hover

4e47a58

Merge branch 'master' into hover

f2c7158

Merge branch 'master' into hover

254b6ce

jeremypress reviewed Apr 19, 2018

View reviewed changes

pramodsum and others added 8 commits April 24, 2018 10:30

Merge branch 'master' into hover

cc3cb5b

Merge branch 'master' into hover

c98f43c

Fix: tests

3e8852b

Chore: Cleanup

721680f

Fix: tests

3c87053

Fix: Tests

cdded80

Merge branch 'master' into hover

aaf712f

Fix: Allow dialogs to trigger consistently w/in modes

944b54b

- Point mode: All point annotations are triggerable - Drawing mode: No existing dialogs are triggerable - When not in a mode: all existing annotation dialogs are triggerable

pramodsum force-pushed the hover branch from d090511 to 944b54b Compare May 15, 2018 20:22

JustinHoldstock approved these changes May 15, 2018

View reviewed changes

jeremypress reviewed May 15, 2018

View reviewed changes

Fix: Tests

bd8ad96

jeremypress reviewed May 15, 2018

View reviewed changes

Chore: Cleanup

8568043

pramodsum force-pushed the hover branch from 4275d65 to 8568043 Compare May 15, 2018 21:37

pramodsum merged commit 4850702 into box:master May 15, 2018

@@ @@ -678,142 +659,6 @@ class DocAnnotator extends Annotator { @@
                       }
                   }
-                  /**

Breaking: Remove hover behavior to display dialogs #155

Breaking: Remove hover behavior to display dialogs #155

Conversation

pramodsum commented Mar 28, 2018 • edited

boxcla commented Mar 28, 2018

jeremypress left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypress May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pramodsum May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypress May 15, 2018 • edited

Choose a reason for hiding this comment

pramodsum commented Mar 28, 2018 •

edited

jeremypress May 15, 2018 •

edited

pramodsum May 15, 2018 •

edited

jeremypress May 15, 2018 •

edited