feat(annotations): add ftux cursor logic #1281

ChenCodes · 2020-11-03T20:02:27Z

Changes in this PR:

add logic to hide popup cursor after the first time that the user explicitly clicks the create region button
hide cursor using styles

Todo:

add tests

Demo:

src/lib/viewers/BaseViewer.js

src/lib/viewers/doc/DocBaseViewer.js

src/lib/viewers/image/ImageViewer.js

jstoffan · 2020-11-03T20:41:03Z

src/lib/viewers/image/ImageViewer.js

+            if (!this.cache.get(IMAGE_FTUX_CURSOR_DISABLED_KEY)) {
+                this.cache.set(IMAGE_FTUX_CURSOR_DISABLED_KEY, true, true /* useLocalStorage */);
+            } else {
+                this.annotator.emit('annotations_image_explicit_create_toggled');


It looks like we're both passing in options to the annotator and emitting events to it. Are they related? Can this be simplified?

They are related:

On a fresh reload of the page, we want to pass the ftux cursor flag via options to the annotator. That will need to be done within initAnnotations. That logic can't be done here because this method is for handling the user click event on the Create Region button.

The first time that the the user clicks on the Create Region button, we want to update the flag within localStorage. The second time that they click on the Create Region button, we no longer want to update the flag (it's already set), but we do want to emit an event that would cause a rerender of RegionCreator.

I'm not aware of how this could be simplified but would be open to suggestions.

We no longer need to pass in options to the annotator after implementing the css approach.

src/lib/viewers/BaseViewer.js

ConradJChan · 2020-11-04T18:34:30Z

src/lib/viewers/doc/DocBaseViewer.js

@@ -1596,6 +1596,11 @@ class DocBaseViewer extends BaseViewer {

    handleAnnotationControlsClick({ mode }) {
        const nextMode = this.annotationControlsFSM.transition(AnnotationInput.CLICK, mode);
+
+        if (nextMode === AnnotationMode.REGION) {


annotationControlsFSM currently subscribes to the transition changes but calls updateDiscoverabilityResinTag directly, perhaps we could introduce a handler that also checks for this condition and applies the class there.

Also it seems like we would want to update localStorage on the transition from REGION to NONE, or even to REGION but only trigger the logic to apply the class on any subsequent transition to REGION

Discussed offline that since the transition changes only happen when the user clicks, we're okay with not needing the handler.

I'm still debating whether this should be moved out to a fsm callback. This method is pretty mode agnostic so I'm wary of cluttering it with some region specific code. Let's do the callback because that'll be easier to strip out eventually too since it'll be it's own separate function. Something like:

this.annotationControlsFSM.subscribe(this.applyCursorFtux);

src/lib/viewers/doc/_docBase.scss

ConradJChan · 2020-11-04T18:35:25Z

src/lib/viewers/BaseViewer.js

+     */
+    handleFtuxCursorToggle(key) {
+        if (!this.cache.get(key)) {
+            this.cache.set(key, true, true);


Also, should we perform this ftux on a per user id basis?

After doing some digging, it looks like we don't need to do it on a per user id basis because localStorage sessions are different for unique users.

ConradJChan · 2020-11-06T23:57:50Z

src/lib/viewers/BaseViewer.js

@@ -162,13 +167,15 @@ class BaseViewer extends EventEmitter {
        this.mobileZoomEndHandler = this.mobileZoomEndHandler.bind(this);
        this.handleAnnotatorEvents = this.handleAnnotatorEvents.bind(this);
        this.handleAnnotationControlsEscape = this.handleAnnotationControlsEscape.bind(this);
+        this.applyCursorFtux = this.applyCursorFtux.bind(this);


Are these binds required? and L178

They weren't required previously, but with the addition of the subscribe call, they are now required.

src/lib/viewers/BaseViewer.js

ConradJChan · 2020-11-07T00:15:02Z

src/lib/viewers/doc/DocBaseViewer.js

@@ -1596,6 +1596,11 @@ class DocBaseViewer extends BaseViewer {

    handleAnnotationControlsClick({ mode }) {
        const nextMode = this.annotationControlsFSM.transition(AnnotationInput.CLICK, mode);
+
+        if (nextMode === AnnotationMode.REGION) {


I'm still debating whether this should be moved out to a fsm callback. This method is pretty mode agnostic so I'm wary of cluttering it with some region specific code. Let's do the callback because that'll be easier to strip out eventually too since it'll be it's own separate function. Something like:

this.annotationControlsFSM.subscribe(this.applyCursorFtux);

ConradJChan · 2020-11-07T00:15:40Z

src/lib/viewers/image/Image.scss

+
+.bp-content {
+    &.bp-annotations-ftux-image-cursor-seen {
+        .bp-image {


Is the .bp-image a needed specificity?

Yes it is required.

What about if we did &.bp-annotations--create-region.bp-annotations-ftux-image-cursor-seen?

src/lib/viewers/BaseViewer.js

ChenCodes · 2020-11-09T17:09:38Z

#1281 (comment)

Yes it's needed.

src/lib/viewers/doc/DocBaseViewer.js

ConradJChan · 2020-11-09T23:58:37Z

src/lib/viewers/doc/_docBase.scss

@@ -370,6 +370,12 @@ $thumbnail-sidebar-width: 226px;
        }
    }

+    &.bp-annotations-ftux-document-cursor-seen {


Could this be moved into &.bp-annotations-create--region above?

Yep this works as well.

ConradJChan · 2020-11-10T00:01:25Z

src/lib/viewers/image/Image.scss

+
+.bp-content {
+    &.bp-annotations-ftux-image-cursor-seen {
+        .bp-image {


What about if we did &.bp-annotations--create-region.bp-annotations-ftux-image-cursor-seen?

Dismissing as this PR has been approved

ChenCodes commented Nov 3, 2020

View reviewed changes

src/lib/viewers/BaseViewer.js Outdated Show resolved Hide resolved

ChenCodes commented Nov 3, 2020

View reviewed changes

src/lib/viewers/doc/DocBaseViewer.js Outdated Show resolved Hide resolved

ChenCodes commented Nov 3, 2020

View reviewed changes

src/lib/viewers/image/ImageViewer.js Outdated Show resolved Hide resolved

jstoffan previously requested changes Nov 3, 2020

View reviewed changes

ChenCodes force-pushed the ftux-cursor-experience branch from 676eb8d to de18983 Compare November 3, 2020 21:56

ChenCodes marked this pull request as ready for review November 4, 2020 18:22

ChenCodes requested a review from a team as a code owner November 4, 2020 18:22

ConradJChan reviewed Nov 4, 2020

View reviewed changes

ConradJChan reviewed Nov 7, 2020

View reviewed changes

jstoffan reviewed Nov 7, 2020

View reviewed changes

src/lib/viewers/BaseViewer.js Outdated Show resolved Hide resolved

src/lib/viewers/BaseViewer.js Outdated Show resolved Hide resolved

ChenCodes added 13 commits November 9, 2020 13:24

feat(annotations): add ftux cursor logic

234223c

fix(annotations): use css logic for hiding the cursor

d0381d0

chore(annotations): remove unused property

1f167b4

chore(annotations): add unit tests

5cffbc5

chore(annotations): remove property from method comments

a7ce55c

chore(annotations): rename as applyCursorFtux

2766a08

chore(annotations): refactor set cursor ftux logic

8c70d77

fix(annotations): move style to Image.scss

ff88cfe

fix(annotations): refactor applyCursorFtux logic

8c21a8a

fix(annotations): remove unnecessary class

27899c6

chore(annotations): move shared logic into subclasses

f3e1155

fix(annotations): bp-image class is required

a53eb41

fix(annotations): use fsm callback

ad04d4c

ChenCodes force-pushed the ftux-cursor-experience branch from 30ceb72 to ad04d4c Compare November 9, 2020 21:30

ChenCodes requested review from ConradJChan and jstoffan November 9, 2020 21:34

ConradJChan reviewed Nov 9, 2020

View reviewed changes

src/lib/viewers/doc/DocBaseViewer.js Outdated Show resolved Hide resolved

src/lib/viewers/doc/DocBaseViewer.js Outdated Show resolved Hide resolved

ConradJChan reviewed Nov 10, 2020

View reviewed changes

fix(annotations): move style logic

1dba9f9

chore(annotations): flatten code logic

034b4f5

ChenCodes requested a review from ConradJChan November 10, 2020 18:52

ConradJChan approved these changes Nov 10, 2020

View reviewed changes

ChenCodes added the ready-to-merge label Nov 10, 2020

Merge branch 'master' into ftux-cursor-experience

f41ae32

ChenCodes merged commit b1a47b4 into box:master Nov 10, 2020

ChenCodes deleted the ftux-cursor-experience branch November 10, 2020 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(annotations): add ftux cursor logic #1281

feat(annotations): add ftux cursor logic #1281

ChenCodes commented Nov 3, 2020 •

edited

Loading

jstoffan Nov 3, 2020

ChenCodes Nov 3, 2020 •

edited

Loading

ChenCodes Nov 4, 2020

ConradJChan Nov 4, 2020

ChenCodes Nov 5, 2020

ConradJChan Nov 7, 2020

ConradJChan Nov 4, 2020

ChenCodes Nov 6, 2020

ConradJChan Nov 6, 2020

ChenCodes Nov 9, 2020

ConradJChan Nov 7, 2020

ConradJChan Nov 7, 2020

ChenCodes Nov 9, 2020

ConradJChan Nov 10, 2020

ChenCodes commented Nov 9, 2020 •

edited

Loading

ConradJChan Nov 9, 2020

ChenCodes Nov 10, 2020

ConradJChan Nov 10, 2020

feat(annotations): add ftux cursor logic #1281

feat(annotations): add ftux cursor logic #1281

Conversation

ChenCodes commented Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

ChenCodes Nov 3, 2020 • edited Loading

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

ChenCodes commented Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenCodes commented Nov 3, 2020 •

edited

Loading

ChenCodes Nov 3, 2020 •

edited

Loading

ChenCodes commented Nov 9, 2020 •

edited

Loading