Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(discoverability): add image cursor experience #1270

Merged
merged 30 commits into from
Oct 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
199fcaf
feat(discoverability): add image cursor experience
ChenCodes Oct 5, 2020
f06b6d6
feat(discoverability): move shared methods / consts to parent class
ChenCodes Oct 5, 2020
5ec96b6
feat(discoverability): pass in initialMode to AnnotationControls
ChenCodes Oct 5, 2020
e0f63c8
feat(discoverability): update ImageViewer-test
ChenCodes Oct 5, 2020
fcfd323
feat(discoverability): remove whitespace / clean up test
ChenCodes Oct 5, 2020
f4a20e1
feat(discoverability): revert moving logic into ImageBaseViewer
ChenCodes Oct 5, 2020
4fa7784
feat(discoverability): add handleZoomEvent listener / put helpers back
ChenCodes Oct 6, 2020
7d4871b
feat(discoverability): revert back to original code ordering
ChenCodes Oct 6, 2020
5c353c0
feat(discoverability): revert test block ordering
ChenCodes Oct 6, 2020
d4d0080
feat(discoverability): move viewport logic into getViewportDimensions
ChenCodes Oct 6, 2020
c6a7647
feat(discoverability): set AnnotationControlsFSM as REGION
ChenCodes Oct 6, 2020
275f74e
feat(discoverability): move zoom event logic into ImageBaseViewer
ChenCodes Oct 6, 2020
f0733f6
feat(discoverability): reorder imports
ChenCodes Oct 6, 2020
dd20098
feat(discoverability): update AnnotationControls test
ChenCodes Oct 6, 2020
7f48fcb
chore(discoverability): move import statement
ChenCodes Oct 6, 2020
2e886d3
feat(discoverability): test cleanup
ChenCodes Oct 6, 2020
98a7b56
feat(discoverability): add destroy method / move zoom logic
ChenCodes Oct 6, 2020
1784cb5
feat(discoverability): always instantiate FSM in ImageViewer
ChenCodes Oct 6, 2020
1f19789
feat(discoverability): use REGION_TEMP instead of REGION
ChenCodes Oct 6, 2020
a9a9ff7
feat(discoverability): call toggleAnnotationMode
ChenCodes Oct 7, 2020
5e069e0
feat(discoverability): add check for current state is not region
ChenCodes Oct 7, 2020
ba28277
feat(discoverability): use getState / update tests
ChenCodes Oct 7, 2020
4d98138
feat(discoverability): add first render check
ChenCodes Oct 7, 2020
20e15ec
feat(discoverability): add noops for annotation events
ChenCodes Oct 7, 2020
be491b2
feat(discoverability): use guard logic
ChenCodes Oct 8, 2020
3353da4
chore(discoverability): lint issues
ChenCodes Oct 20, 2020
cfab871
fix(discoverability): remove noop methods
ChenCodes Oct 20, 2020
0b68ad0
chore(discoverability): update tests
ChenCodes Oct 20, 2020
b29629c
chore(discoverability): refactor short circuit logic
ChenCodes Oct 20, 2020
7fa2915
chore(discoverability): use .each
ChenCodes Oct 20, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/lib/AnnotationControls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export enum AnnotationMode {
export type ClickHandler = ({ event, mode }: { event: MouseEvent; mode: AnnotationMode }) => void;
export type Options = {
fileId: string;
initialMode?: AnnotationMode;
onClick?: ClickHandler;
onEscape?: () => void;
showHighlightText: boolean;
Expand Down Expand Up @@ -194,7 +195,13 @@ export default class AnnotationControls {
/**
* Initialize the annotation controls with options.
*/
public init({ fileId, onEscape = noop, onClick = noop, showHighlightText = false }: Options): void {
public init({
fileId,
initialMode = AnnotationMode.NONE,
onEscape = noop,
onClick = noop,
showHighlightText = false,
}: Options): void {
if (this.hasInit) {
return;
}
Expand All @@ -210,5 +217,7 @@ export default class AnnotationControls {
document.addEventListener('keydown', this.handleKeyDown);

this.hasInit = true;

this.setMode(initialMode);
}
}
2 changes: 2 additions & 0 deletions src/lib/Preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,8 @@ class Preview extends EventEmitter {

this.options.enableAnnotationsDiscoverability = !!options.enableAnnotationsDiscoverability;

this.options.enableAnnotationsImageDiscoverability = !!options.enableAnnotationsImageDiscoverability;

// Enable or disable hotkeys
this.options.useHotkeys = options.useHotkeys !== false;

Expand Down
6 changes: 6 additions & 0 deletions src/lib/__tests__/AnnotationControls-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ describe('lib/AnnotationControls', () => {
expect(annotationControls.addButton).not.toBeCalled();
expect(document.addEventListener).not.toBeCalled();
});

test('should set annotationControls currentMode to be REGION', () => {
annotationControls.init({ initialMode: AnnotationMode.REGION });

expect(annotationControls.currentMode).toBe(AnnotationMode.REGION);
});
});

describe('handleKeyDown', () => {
Expand Down
71 changes: 65 additions & 6 deletions src/lib/viewers/image/ImageViewer.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import AnnotationControls from '../../AnnotationControls';
import ImageBaseViewer from './ImageBaseViewer';
import { AnnotationInput } from '../../AnnotationControlsFSM';
import AnnotationControls, { AnnotationMode } from '../../AnnotationControls';
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
import AnnotationControlsFSM, { AnnotationInput, AnnotationState, stateModeMap } from '../../AnnotationControlsFSM';
import { CLASS_INVISIBLE } from '../../constants';
import { ICON_FULLSCREEN_IN, ICON_FULLSCREEN_OUT, ICON_ROTATE_LEFT } from '../../icons/icons';
import './Image.scss';
Expand All @@ -19,12 +19,30 @@ class ImageViewer extends ImageBaseViewer {
this.handleAnnotationControlsClick = this.handleAnnotationControlsClick.bind(this);
this.handleAssetAndRepLoad = this.handleAssetAndRepLoad.bind(this);
this.handleImageDownloadError = this.handleImageDownloadError.bind(this);
this.getViewportDimensions = this.getViewportDimensions.bind(this);
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
this.handleZoomEvent = this.handleZoomEvent.bind(this);
this.annotationControlsFSM = new AnnotationControlsFSM(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ChenCodes ChenCodes Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

this.options.enableAnnotationsImageDiscoverability ? AnnotationState.REGION_TEMP : AnnotationState.NONE,
);

if (this.isMobile) {
this.handleOrientationChange = this.handleOrientationChange.bind(this);
}
}

/**
* [destructor]
*
* @return {void}
*/
destroy() {
if (this.options.enableAnnotationsImageDiscoverability) {
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
this.removeListener('zoom', this.handleZoomEvent);
}

super.destroy();
}

/**
* @inheritdoc
*/
Expand All @@ -46,6 +64,10 @@ class ImageViewer extends ImageBaseViewer {
this.imageEl.classList.add(CLASS_INVISIBLE);

this.currentRotationAngle = 0;

if (this.options.enableAnnotationsImageDiscoverability) {
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
this.addListener('zoom', this.handleZoomEvent);
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
Expand Down Expand Up @@ -167,6 +189,41 @@ class ImageViewer extends ImageBaseViewer {
};
}

/**
* Gets the viewport dimensions.
*
* @return {Object} the width & height of the viewport
*/
getViewportDimensions() {
return {
width: this.wrapperEl.clientWidth - 2 * IMAGE_PADDING,
height: this.wrapperEl.clientHeight - 2 * IMAGE_PADDING,
};
}

/**
* Sets mode to be AnnotationMode.NONE if the zoomed image overflows the viewport.
*
* @return {void}
*/
handleZoomEvent({ newScale, type }) {
const [width, height] = newScale;

// type is undefined on initial render, we only want below logic to execute on user initiated actions
if (!type) {
return;
}

const viewport = this.getViewportDimensions();
mxiao6 marked this conversation as resolved.
Show resolved Hide resolved

// We only set AnnotationMode to be NONE if the image overflows the viewport and the state is not explicit region creation
const currentState = this.annotationControlsFSM.getState();
if (currentState === AnnotationState.REGION_TEMP && (width > viewport.width || height > viewport.height)) {
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
this.annotator.toggleAnnotationMode(AnnotationMode.NONE);
this.processAnnotationModeChange(this.annotationControlsFSM.transition(AnnotationInput.CANCEL));
}
}

/**
* Handles zoom
*
Expand Down Expand Up @@ -202,10 +259,8 @@ class ImageViewer extends ImageBaseViewer {
({ width, height } = this.getTransformWidthAndHeight(origWidth, origHeight, isRotated));
const modifyWidthInsteadOfHeight = width >= height;

const viewport = {
width: this.wrapperEl.clientWidth - 2 * IMAGE_PADDING,
height: this.wrapperEl.clientHeight - 2 * IMAGE_PADDING,
};
const viewport = this.getViewportDimensions();

// If the image is overflowing the viewport, figure out by how much
// Then take that aspect that reduces the image the maximum (hence min ratio) to fit both width and height
if (width > viewport.width || height > viewport.height) {
Expand Down Expand Up @@ -251,6 +306,7 @@ class ImageViewer extends ImageBaseViewer {
newScale: [newWidth || width, newHeight || height],
canZoomIn: true,
canZoomOut: true,
type,
});
}

Expand Down Expand Up @@ -299,6 +355,9 @@ class ImageViewer extends ImageBaseViewer {
this.annotationControls = new AnnotationControls(this.controls);
this.annotationControls.init({
fileId: this.options.file.id,
initialMode: this.options.enableAnnotationsImageDiscoverability
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
? stateModeMap[AnnotationState.REGION_TEMP]
: stateModeMap[AnnotationState.NONE],
onClick: this.handleAnnotationControlsClick,
onEscape: this.handleAnnotationControlsEscape,
});
Expand Down
131 changes: 118 additions & 13 deletions src/lib/viewers/image/__tests__/ImageViewer-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable no-unused-expressions */
import AnnotationControls, { AnnotationMode } from '../../../AnnotationControls';
import AnnotationControlsFSM, { AnnotationState, stateModeMap } from '../../../AnnotationControlsFSM';
import ImageViewer from '../ImageViewer';
import BaseViewer from '../../BaseViewer';
import Browser from '../../../Browser';
Expand Down Expand Up @@ -52,11 +53,45 @@ describe('lib/viewers/image/ImageViewer', () => {
stubs = {};
});

describe('destroy()', () => {
test.each`
enableAnnotationsImageDiscoverability | numberOfCalls
${true} | ${1}
${false} | ${0}
`(
'should call removeListener $numberOfCalls times if enableAnnotationsImageDiscoverability is $enableAnnotationsImageDiscoverability',
({ enableAnnotationsImageDiscoverability, numberOfCalls }) => {
image.options.enableAnnotationsImageDiscoverability = enableAnnotationsImageDiscoverability;
jest.spyOn(image, 'removeListener');

image.destroy();

expect(image.removeListener).toBeCalledTimes(numberOfCalls);
},
);
});

describe('setup()', () => {
test('should set up layout', () => {
expect(image.wrapperEl).toHaveClass('bp-image');
expect(image.imageEl).toHaveClass('bp-is-invisible');
});

test.each`
enableAnnotationsImageDiscoverability | numberOfCalls
${true} | ${1}
${false} | ${0}
`(
'should call addListener $numberOfCalls times if enableAnnotationsImageDiscoverability is $enableAnnotationsImageDiscoverability',
({ enableAnnotationsImageDiscoverability, numberOfCalls }) => {
image.options.enableAnnotationsImageDiscoverability = enableAnnotationsImageDiscoverability;
jest.spyOn(image, 'addListener');

image.setup();

expect(image.addListener).toBeCalledTimes(numberOfCalls);
},
);
});

describe('load()', () => {
Expand Down Expand Up @@ -351,19 +386,28 @@ describe('lib/viewers/image/ImageViewer', () => {
expect(image.annotationControls).toBeInstanceOf(AnnotationControls);
});

test('should call annotations controls init with callbacks', () => {
jest.spyOn(image, 'areNewAnnotationsEnabled').mockReturnValue(true);
jest.spyOn(image, 'hasAnnotationCreatePermission').mockReturnValue(true);
jest.spyOn(AnnotationControls.prototype, 'init').mockImplementation();

image.loadUI();

expect(AnnotationControls.prototype.init).toBeCalledWith({
fileId: image.options.file.id,
onClick: image.handleAnnotationControlsClick,
onEscape: image.handleAnnotationControlsEscape,
});
});
test.each`
enableAnnotationsImageDiscoverability | initialMode
${false} | ${stateModeMap[AnnotationState.NONE]}
${true} | ${stateModeMap[AnnotationState.REGION_TEMP]}
`(
'should call annotation controls init with $initialMode when enableAnnotationsImageDiscoverability is $enableAnnotationsImageDiscoverability',
({ enableAnnotationsImageDiscoverability, initialMode }) => {
image.options.enableAnnotationsImageDiscoverability = enableAnnotationsImageDiscoverability;
jest.spyOn(image, 'areNewAnnotationsEnabled').mockReturnValue(true);
jest.spyOn(image, 'hasAnnotationCreatePermission').mockReturnValue(true);
jest.spyOn(AnnotationControls.prototype, 'init').mockImplementation();

image.loadUI();

expect(AnnotationControls.prototype.init).toBeCalledWith({
fileId: image.options.file.id,
initialMode,
onClick: image.handleAnnotationControlsClick,
onEscape: image.handleAnnotationControlsEscape,
});
},
);
});

describe('isRotated()', () => {
Expand Down Expand Up @@ -597,4 +641,65 @@ describe('lib/viewers/image/ImageViewer', () => {
expect(image.processAnnotationModeChange).toBeCalledWith(AnnotationMode.NONE);
});
});

describe('getViewportDimensions', () => {
test('should return width and height', () => {
image.wrapperEl = document.createElement('img');
Object.defineProperty(image.wrapperEl, 'clientWidth', { value: 100 });
Object.defineProperty(image.wrapperEl, 'clientHeight', { value: 100 });

const result = image.getViewportDimensions();

expect(result).toEqual({ width: 70, height: 70 });
});
});

describe('handleZoomEvent', () => {
beforeEach(() => {
image.wrapperEl = document.createElement('img');
Object.defineProperty(image.wrapperEl, 'clientWidth', { value: 100 });
Object.defineProperty(image.wrapperEl, 'clientHeight', { value: 100 });
jest.spyOn(image, 'processAnnotationModeChange');
});

test('should not call getViewportDimensions if type is undefined', () => {
const width = 100;
const height = 100;
image.getViewportDimensions = jest.fn();

image.handleZoomEvent({ newScale: [width, height], type: undefined });

expect(image.getViewportDimensions).not.toHaveBeenCalled();
});

test.each`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

currentState | height | width
${AnnotationState.REGION} | ${110} | ${110}
${AnnotationState.REGION} | ${60} | ${60}
${AnnotationState.REGION_TEMP} | ${60} | ${60}
`(
'should not call processAnnotationModeChange when height is $height and width is $width and currentState is $currentState',
({ currentState, height, width }) => {
image.annotationControlsFSM = new AnnotationControlsFSM(currentState);

image.handleZoomEvent({ newScale: [width, height], type: 'in' });

expect(image.processAnnotationModeChange).not.toHaveBeenCalled();
},
);

test('should call processAnnotationModeChange and toggleAnnotationMode if image does overflow the viewport and currentState is REGION_TEMP', () => {
const width = 110;
const height = 110;
image.annotator = {
toggleAnnotationMode: jest.fn(),
};
image.annotationControlsFSM = new AnnotationControlsFSM(AnnotationState.REGION_TEMP);

image.handleZoomEvent({ newScale: [width, height], type: 'in' });

expect(image.processAnnotationModeChange).toHaveBeenCalled();
expect(image.annotator.toggleAnnotationMode).toHaveBeenCalled();
});
});
});