feat(discoverability): add image cursor experience #1270

ChenCodes · 2020-10-05T17:47:02Z

Changes in this PR:

store FF in options in Preview.js
add FF into init method for AnnotationControls
add logic for turning button mode to be off when the the image overflows the viewport after zoom
add unit tests

Demo:

src/lib/viewers/image/ImageViewer.js

src/lib/AnnotationControls.ts

src/lib/viewers/image/ImageViewer.js

src/lib/viewers/image/ImageBaseViewer.js

src/lib/__tests__/AnnotationControls-test.js

src/lib/viewers/image/ImageBaseViewer.js

src/lib/viewers/image/ImageViewer.js

src/lib/viewers/image/__tests__/ImageViewer-test.js

src/lib/__tests__/AnnotationControls-test.js

src/lib/viewers/image/ImageBaseViewer.js

src/lib/viewers/image/ImageViewer.js

src/lib/viewers/image/__tests__/ImageViewer-test.js

mxiao6 · 2020-10-07T21:32:05Z

src/lib/viewers/image/ImageViewer.js

@@ -17,12 +18,29 @@ class ImageViewer extends ImageBaseViewer {
        this.updatePannability = this.updatePannability.bind(this);
        this.handleImageDownloadError = this.handleImageDownloadError.bind(this);
        this.handleAssetAndRepLoad = this.handleAssetAndRepLoad.bind(this);
+        this.handleZoomEvent = this.handleZoomEvent.bind(this);
+        this.annotationControlsFSM = new AnnotationControlsFSM(


AnnotationControlsFSM has already been created in BaseViewer constructor, right? Should we move the logic to BaseViewer? Or call something like this.annotationControlsFSM.setState here?

I think @ConradJChan and I discussed that we wanted ImageViewer to instantiate its own instance of AnnotationControlsFSM.

Does it need to re-instantiate when enableAnnotationsImageDiscoverability === false? How about:

if (this.options.enableAnnotationsImageDiscoverability) { this.annotationControlsFSM = new AnnotationControlsFSM(AnnotationState.REGION_TEMP); }

Let's also make sure that if enableAnnotationsImageDiscoverability === false, that we get the old ImageViewer experience, i.e. not the enableAnnotationsDiscoverability experience

Could you elaborate a little on this? Do you mean that we should have a check like this:

if (enableAnnotationsImageDiscoverability) { this.annotationControlsFSM = new AnnotationControlsFSM(AnnotationState.REGION_TEMP); }

src/lib/viewers/image/ImageViewer.js

ConradJChan · 2020-10-07T23:01:40Z

src/lib/viewers/image/__tests__/ImageViewer-test.js

+            jest.spyOn(image, 'processAnnotationModeChange');
+        });
+
+        test.each`


Seems like this test may be combining too many things. Maybe split it out to a test based on type to make sure it doesn't get processed and the another test for the combinations of currentState and whether the image overflows

The should param is probably unnecessary. Maybe just update the test description to include the height and width, otherwise consider breaking out a separate test to test the currentState vs overflow conditions

mxiao6 · 2020-10-08T20:44:35Z

src/lib/viewers/image/ImageViewer.js

@@ -17,12 +18,29 @@ class ImageViewer extends ImageBaseViewer {
        this.updatePannability = this.updatePannability.bind(this);
        this.handleImageDownloadError = this.handleImageDownloadError.bind(this);
        this.handleAssetAndRepLoad = this.handleAssetAndRepLoad.bind(this);
+        this.handleZoomEvent = this.handleZoomEvent.bind(this);
+        this.annotationControlsFSM = new AnnotationControlsFSM(


Does it need to re-instantiate when enableAnnotationsImageDiscoverability === false? How about:

if (this.options.enableAnnotationsImageDiscoverability) { this.annotationControlsFSM = new AnnotationControlsFSM(AnnotationState.REGION_TEMP); }

src/lib/viewers/image/ImageViewer.js

src/lib/viewers/image/__tests__/ImageViewer-test.js

ConradJChan · 2020-10-20T20:42:18Z

src/lib/viewers/image/__tests__/ImageViewer-test.js

+            jest.spyOn(image, 'processAnnotationModeChange');
+        });
+
+        test.each`


The should param is probably unnecessary. Maybe just update the test description to include the height and width, otherwise consider breaking out a separate test to test the currentState vs overflow conditions

src/lib/viewers/image/__tests__/ImageViewer-test.js

ChenCodes commented Oct 5, 2020

View reviewed changes

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

ChenCodes requested review from mxiao6, jstoffan and ConradJChan and removed request for mxiao6 and jstoffan October 5, 2020 17:49

ConradJChan reviewed Oct 5, 2020

View reviewed changes

src/lib/AnnotationControls.ts Outdated Show resolved Hide resolved

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

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

ChenCodes commented Oct 5, 2020

View reviewed changes

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

ChenCodes requested a review from ConradJChan October 5, 2020 22:11

ConradJChan reviewed Oct 6, 2020

View reviewed changes

ChenCodes requested a review from ConradJChan October 6, 2020 17:16

ConradJChan reviewed Oct 6, 2020

View reviewed changes

ChenCodes requested a review from ConradJChan October 6, 2020 18:31

ChenCodes marked this pull request as ready for review October 6, 2020 19:29

ChenCodes requested a review from a team as a code owner October 6, 2020 19:29

ConradJChan reviewed Oct 6, 2020

View reviewed changes

mxiao6 reviewed Oct 6, 2020

View reviewed changes

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

ChenCodes requested a review from ConradJChan October 7, 2020 18:04

ConradJChan reviewed Oct 7, 2020

View reviewed changes

mxiao6 reviewed Oct 7, 2020

View reviewed changes

ChenCodes requested a review from ConradJChan October 7, 2020 22:52

ConradJChan reviewed Oct 7, 2020

View reviewed changes

ChenCodes requested a review from ConradJChan October 8, 2020 00:30

mxiao6 reviewed Oct 8, 2020

View reviewed changes

ChenCodes added 4 commits October 20, 2020 11:40

feat(discoverability): add image cursor experience

199fcaf

feat(discoverability): move shared methods / consts to parent class

f06b6d6

feat(discoverability): pass in initialMode to AnnotationControls

5ec96b6

feat(discoverability): update ImageViewer-test

e0f63c8

ChenCodes added 17 commits October 20, 2020 11:49

feat(discoverability): move viewport logic into getViewportDimensions

d4d0080

feat(discoverability): set AnnotationControlsFSM as REGION

c6a7647

feat(discoverability): move zoom event logic into ImageBaseViewer

275f74e

feat(discoverability): reorder imports

f0733f6

feat(discoverability): update AnnotationControls test

dd20098

chore(discoverability): move import statement

7f48fcb

feat(discoverability): test cleanup

2e886d3

feat(discoverability): add destroy method / move zoom logic

98a7b56

feat(discoverability): always instantiate FSM in ImageViewer

1784cb5

feat(discoverability): use REGION_TEMP instead of REGION

1f19789

feat(discoverability): call toggleAnnotationMode

a9a9ff7

feat(discoverability): add check for current state is not region

5e069e0

feat(discoverability): use getState / update tests

ba28277

feat(discoverability): add first render check

4d98138

feat(discoverability): add noops for annotation events

20e15ec

feat(discoverability): use guard logic

be491b2

chore(discoverability): lint issues

3353da4

ChenCodes force-pushed the add-image-discoverability-experience branch from 5af3073 to 3353da4 Compare October 20, 2020 19:55

fix(discoverability): remove noop methods

cfab871

ConradJChan reviewed Oct 20, 2020

View reviewed changes

ChenCodes added 2 commits October 20, 2020 14:08

chore(discoverability): update tests

0b68ad0

chore(discoverability): refactor short circuit logic

b29629c

ConradJChan previously approved these changes Oct 20, 2020

View reviewed changes

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

chore(discoverability): use .each

7fa2915

ChenCodes dismissed ConradJChan’s stale review via 7fa2915 October 20, 2020 22:44

ChenCodes requested a review from ConradJChan October 20, 2020 22:45

ConradJChan approved these changes Oct 20, 2020

View reviewed changes

ChenCodes added the ready-to-merge label Oct 20, 2020

ChenCodes merged commit b42768a into box:master Oct 20, 2020

ChenCodes deleted the add-image-discoverability-experience branch October 20, 2020 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(discoverability): add image cursor experience #1270

feat(discoverability): add image cursor experience #1270

ChenCodes commented Oct 5, 2020 •

edited

Loading

mxiao6 Oct 7, 2020

ChenCodes Oct 7, 2020

mxiao6 Oct 8, 2020

ConradJChan Oct 9, 2020

ChenCodes Oct 20, 2020 •

edited

Loading

ConradJChan Oct 7, 2020

ConradJChan Oct 20, 2020

mxiao6 Oct 8, 2020

ConradJChan Oct 20, 2020

feat(discoverability): add image cursor experience #1270

feat(discoverability): add image cursor experience #1270

Conversation

ChenCodes commented Oct 5, 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

ChenCodes Oct 20, 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

ChenCodes commented Oct 5, 2020 •

edited

Loading

ChenCodes Oct 20, 2020 •

edited

Loading