Temp hide show ui for plain and comment highlights #348

JustinHoldstock · 2017-08-29T22:52:10Z

This change hides UI elements in the CRUD process of Plain and Comment highlight annotations.

THIS IS A TEMPORARY CHANGE UNTIL WE CAN PROPERLY REFACTOR COMMENT AND PLAIN HIGHLIGHTS

As always, tests to come :)

pramodsum · 2017-08-30T02:10:11Z

src/lib/annotations/BoxAnnotations.js

@@ -7,7 +7,7 @@ const ANNOTATORS = [
        NAME: 'Document',
        CONSTRUCTOR: DocAnnotator,
        VIEWER: ['Document', 'Presentation'],
-        TYPE: [TYPES.point, TYPES.highlight]
+        TYPE: [TYPES.point, TYPES.highlight, TYPES.highlight_comment]


pramodsum · 2017-08-30T02:11:40Z

src/lib/annotations/doc/CreateHighlightDialog.js

@@ -315,6 +312,39 @@ class CreateHighlightDialog extends EventEmitter {
     * @return {HTMLElement} The element containing Highlight creation UI
     */
    createElement() {
+        const caretTemplate = this.isMobile


should we move these templates out into constants somewhere?

That would've made more sense than what I did :D DONE!

pramodsum · 2017-08-30T02:15:29Z

src/lib/annotations/doc/CreateHighlightDialog.js

+            this.highlightCreateEl.addEventListener('click', this.onHighlightClick);
+
+            if (this.hasTouch) {
+                this.highlightCreateEl.addEventListener('touchstart', this.stopPropagation);


maybe move the adding of these 2 event handlers into its own method? so you can just call it anywhere on w/e element you pass in?

I'm not sure this'd be valid for the touchend -> highlightClick event binding, as it only ever happens once. Would it be valid abstracting the binding for stopPropagation()?

Maybe instead of

element.addEventListener('event', this.stopPropagation);

We'd do

this.bindStopPropagation(element, event);

Though, after seeing it, I'm not sure what the benefit of that is

hmm true. I guess it wouldn't really clean up too much.

pramodsum · 2017-08-30T02:18:55Z

src/lib/annotations/doc/DocAnnotator.js

+        if (this.commentHighlightEnabled) {
+            this.highlightCurrentSelection = this.highlightCurrentSelection.bind(this);
+            this.createHighlightDialog.addListener(CreateEvents.comment, this.highlightCurrentSelection);
+            this.createHighlightThread = this.createHighlightThread.bind(this);


new line before this

pramodsum · 2017-08-30T02:31:09Z

src/lib/annotations/doc/DocAnnotator.js

@@ -390,7 +423,17 @@ class DocAnnotator extends Annotator {
        // Scale existing canvases on re-render
        this.scaleAnnotationCanvases(pageNum);

-        super.renderAnnotationsOnPage(pageNum);
+        if (this.threads && this.threads[pageNum]) {


Actually if you just leave the call to super.renderAnnotationsOnPage(pageNum)
We can modify annotator.renderAnnotationsOnPage() to this since w/e annotation types aren't supported will get filtered out w/ the call to this.isModeAnnotatable(thread.type)

renderAnnotationsOnPage(pageNum) { if (this.threads && this.threads[pageNum]) { this.threads[pageNum].forEach((thread) => { if (!this.isModeAnnotatable(thread.type)) { return; } thread.show(); }); } }

Plus then it'll be applicable to any other annotation types we don't want to render :)

Check if there's anywhere else we can just make calls to isModeAnnotatable() to Annotator or AnnotationThread methods to clean up some of hackier logic

I'll override the super call to do this, but I still have to not call it, as we're calling our butchered version of show() that takes in the flags for plain and comment highlights

I'll make a ticket for the cleanup, as I don't want this task to turn into a refactor/cleanup PR. Then we can do it, next

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

…ghlights

Minh-Ng · 2017-09-01T17:29:14Z

src/lib/annotations/doc/DocAnnotator.js

@@ -751,7 +800,7 @@ class DocAnnotator extends Annotator {
        // hovered over at the same time, only the top-most highlight
        // dialog will be displayed and the others will be hidden
        // without delay
-        delayThreads.forEach(showFirstDialogFilter);
+        delayThreads.forEach(showFirstDialogFilter.bind(this));


is this able to be bound elsewhere?

…github.com:JustinHoldstock/box-content-preview into Temp-hide-show-ui-for-plain-and-comment-highlights

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

…github.com:JustinHoldstock/box-content-preview into Temp-hide-show-ui-for-plain-and-comment-highlights

…ghlights

pramodsum

There will be merge conflicts from #316 and #358. Let me know if you have any questions about those conflicts!

pramodsum · 2017-09-05T19:16:18Z

src/lib/annotations/doc/CreateHighlightDialog.js

+            this.commentBox = new CommentBox(containerEl);
+            this.commentCreateEl = containerEl.querySelector(`.${constants.CLASS_ADD_HIGHLIGHT_COMMENT_BTN}`);
+            this.commentCreateEl.addEventListener('click', this.onCommentClick);
+            // Event listeners


new line above comment

pramodsum · 2017-09-05T19:16:28Z

src/lib/annotations/doc/CreateHighlightDialog.js

+            // Event listeners
+            this.commentBox.addListener(CommentBox.CommentEvents.post, this.onCommentPost);
+            this.commentBox.addListener(CommentBox.CommentEvents.cancel, this.onCommentCancel);
+            // Hide comment box, by default


new line before comment

pramodsum · 2017-09-05T19:17:40Z

src/lib/annotations/doc/DocAnnotator.js

@@ -122,12 +149,19 @@ class DocAnnotator extends Annotator {
     */
    destroy() {
        super.destroy();
+        if (this.createHighlightDialog) {


if (!this.createHighlightDialog) { return; }

pramodsum · 2017-09-05T19:19:36Z

src/lib/annotations/doc/DocAnnotator.js

@@ -485,7 +542,11 @@ class DocAnnotator extends Annotator {
            this.highlightThrottleHandle = null;
        }

-        if (this.hasTouch && this.isMobile) {
+        if (!this.canAnnotate) {


This is now stored as this.permissions.canAnnotate. Update this around the code! See https://github.com/box/box-content-preview/blob/master/src/lib/annotations/Annotator.js#L70

pramodsum · 2017-09-05T19:20:55Z

src/lib/annotations/doc/DocHighlightDialog.js

+        if (showPlain) {
+            annotatorUtil.showElement(plainButtonEl);
+        } else {
+            annotatorUtil.hideElement(plainButtonEl);


You wouldn't ever show the plain highlight dialog AND the comments dialog correct? Can we combine these 2 if/else statements?

This code is for only showing/hiding the buttons

ah right not showing/hiding specific types of dialogs. got it

pramodsum

Need to sort through merge conflicts from #316 and #358.

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

pramodsum · 2017-09-05T20:47:09Z

src/lib/annotations/doc/DocAnnotator.js

@@ -485,6 +547,10 @@ class DocAnnotator extends Annotator {
            this.highlightThrottleHandle = null;
        }

+        if (!this.permissions.canAnnotate) {


i don't think this is needed?

I'd say it's unnecessary to remove it, as we don't want to be removing event listeners that don't exist. I'm not sure what our standard is for that. @jeremypress @tonyjin @minhhnguyen Thoughts?

It's safe to remove event listeners that don't exist. I'd personally prefer fewer lines of code over adding a guard before removing event listeners but I don't think this is something we need to standardize on.

pramodsum · 2017-09-05T20:47:35Z

src/lib/annotations/doc/DocHighlightDialog.js

+        if (showPlain) {
+            annotatorUtil.showElement(plainButtonEl);
+        } else {
+            annotatorUtil.hideElement(plainButtonEl);


ah right not showing/hiding specific types of dialogs. got it

pramodsum · 2017-09-05T20:49:01Z

src/lib/annotations/doc/DocAnnotator.js

+        }
+
+        // TODO (@jholdstock|@spramod) remove this if statement, and make super call, upon refactor.
+        const pageThreads = this.getThreadsOnPage(pageNum);


at this point, isn't this the same as calling super.renderAnnotationsOnPage()? Since that also checks isModeAnnotatable(type) and that's essentially when this.plainHighlightEnabled and this.commentHighlightEnabled is?

Note that the override is passing in the two flags, which are not done in the parent class

thread.show(this.plainHighlightEnabled, this.commentHighlightEnabled);

Conflicts were easy peasy!

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

Justin Holdstock added 2 commits August 29, 2017 15:48

New: Temporary checks to hide ui base on disabled type

650591d

Chore: Fixed warning

2b38cc9

pramodsum reviewed Aug 30, 2017

View reviewed changes

Justin Holdstock added 4 commits August 30, 2017 10:23

Chore: feedback on PR

1503281

Chore: Styling for mobile UI on disabled types

d374903

Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…

a2f21bc

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

Chore: linter warnings

88c92fb

pramodsum approved these changes Aug 31, 2017

View reviewed changes

Justin Holdstock and others added 3 commits August 31, 2017 16:47

Chore: Tests for highlight enabled/disabled guards

4b8e2d2

Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…

ad2f66b

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

Merge branch 'master' into Temp-hide-show-ui-for-plain-and-comment-hi…

28c649e

…ghlights

Minh-Ng reviewed Sep 1, 2017

View reviewed changes

Justin Holdstock and others added 12 commits September 1, 2017 15:06

Chore: Change binding of filter

52b6d45

Merge branch 'Temp-hide-show-ui-for-plain-and-comment-highlights' of …

21c8c70

…github.com:JustinHoldstock/box-content-preview into Temp-hide-show-ui-for-plain-and-comment-highlights

Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…

0062a42

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

New: Temporary checks to hide ui base on disabled type

f529a37

Chore: Fixed warning

e6da085

Chore: feedback on PR

2aa6104

Chore: Styling for mobile UI on disabled types

9d5e0f8

Chore: linter warnings

a2c63a0

Chore: Tests for highlight enabled/disabled guards

cf9c347

Chore: Change binding of filter

840246d

Merge branch 'Temp-hide-show-ui-for-plain-and-comment-highlights' of …

eb2c45c

…github.com:JustinHoldstock/box-content-preview into Temp-hide-show-ui-for-plain-and-comment-highlights

Merge branch 'master' into Temp-hide-show-ui-for-plain-and-comment-hi…

00909f5

…ghlights

pramodsum reviewed Sep 5, 2017

View reviewed changes

pramodsum previously requested changes Sep 5, 2017

View reviewed changes

Justin Holdstock added 4 commits September 5, 2017 12:45

Chore: fixed tests

7bc4a32

Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…

c52901f

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

Chore: forgot to save before commit

ba00e80

Chore: Feedback

daca4a0

Chore: missing test fix

99bd679

pramodsum reviewed Sep 5, 2017

View reviewed changes

Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…

b6c1705

…ew into Temp-hide-show-ui-for-plain-and-comment-highlights

pramodsum approved these changes Sep 5, 2017

View reviewed changes

JustinHoldstock merged commit 3dbddbf into box:master Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temp hide show ui for plain and comment highlights #348

Temp hide show ui for plain and comment highlights #348

JustinHoldstock commented Aug 29, 2017

pramodsum Aug 30, 2017

pramodsum Aug 30, 2017

JustinHoldstock Aug 30, 2017

pramodsum Aug 30, 2017

JustinHoldstock Aug 30, 2017

pramodsum Aug 31, 2017

pramodsum Aug 30, 2017

JustinHoldstock Aug 30, 2017

pramodsum Aug 30, 2017

pramodsum Aug 30, 2017

JustinHoldstock Aug 30, 2017

JustinHoldstock Aug 30, 2017

Minh-Ng Sep 1, 2017

pramodsum left a comment

pramodsum Sep 5, 2017

JustinHoldstock Sep 5, 2017

pramodsum Sep 5, 2017

JustinHoldstock Sep 5, 2017

pramodsum Sep 5, 2017

pramodsum Sep 5, 2017

pramodsum Sep 5, 2017

JustinHoldstock Sep 5, 2017 •

edited

Loading

pramodsum Sep 5, 2017

pramodsum left a comment

pramodsum Sep 5, 2017

JustinHoldstock Sep 5, 2017

tonyjin Sep 5, 2017

pramodsum Sep 5, 2017

pramodsum Sep 5, 2017

JustinHoldstock Sep 5, 2017

Temp hide show ui for plain and comment highlights #348

Temp hide show ui for plain and comment highlights #348

Conversation

JustinHoldstock commented Aug 29, 2017

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

pramodsum 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

JustinHoldstock Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pramodsum 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

JustinHoldstock Sep 5, 2017 •

edited

Loading