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.

Sign up for GitHub

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

Jump to bottom

feat(discoverability): add image cursor experience #1270

Merged
merged 30 commits into from
Oct 20, 2020
Merged

feat(discoverability): add image cursor experience #1270

Show file tree
Hide file tree
Changes from 2 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
5 changes: 3 additions & 2 deletions src/lib/viewers/image/ImageViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,9 @@ class ImageViewer extends ImageBaseViewer {
*/
handleZoomEvent({ newScale, type }) {
const [width, height] = newScale;
const isUserInitiated = type !== undefined;
if (!isUserInitiated) {

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

Expand Down
38 changes: 30 additions & 8 deletions src/lib/viewers/image/__tests__/ImageViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,30 +54,52 @@ describe('lib/viewers/image/ImageViewer', () => {
});

describe('destroy()', () => {
test('should remove the zoom event listener', () => {
beforeEach(() => {
jest.spyOn(image, 'removeListener');
});

test('should remove the zoom event listener if enableAnnotationsImageDiscoverability is true', () => {
image.options.enableAnnotationsImageDiscoverability = true;
image.removeListener = jest.fn();

image.destroy();

expect(image.removeListener).toBeCalledWith('zoom', expect.any(Function));
});

test('should not remove the zoom event listener if enableAnnotationsImageDiscoverability is false', () => {
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
image.options.enableAnnotationsImageDiscoverability = false;

image.destroy();

expect(image.removeListener).not.toBeCalledWith('zoom', expect.any(Function));
});
});

describe('setup()', () => {
beforeEach(() => {
jest.spyOn(image, 'addListener');
});

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

test('should bind zoom listener if enableAnnotationsImageDiscoverability is true', () => {
ChenCodes marked this conversation as resolved.
Show resolved Hide resolved
image.options.enableAnnotationsImageDiscoverability = true;
image.addListener = jest.fn();

image.setup();

expect(image.addListener).toBeCalledWith('zoom', expect.any(Function));
});

test('should not bind zoom listener if enableAnnotationsImageDiscoverability is false', () => {
image.options.enableAnnotationsImageDiscoverability = false;

image.setup();

expect(image.addListener).not.toBeCalledWith('zoom', expect.any(Function));
});
});

describe('load()', () => {
Expand Down Expand Up @@ -659,12 +681,12 @@ describe('lib/viewers/image/ImageViewer', () => {
});

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 | should
${AnnotationState.REGION} | ${110} | ${110} | ${'image does overflow the viewport'}
${AnnotationState.REGION} | ${60} | ${60} | ${'image does not overflow the viewport'}
${AnnotationState.REGION_TEMP} | ${60} | ${60} | ${'image does not overflow the viewport'}
currentState | height | width
${AnnotationState.REGION} | ${110} | ${110}
${AnnotationState.REGION} | ${60} | ${60}
${AnnotationState.REGION_TEMP} | ${60} | ${60}
`(
'should not call processAnnotationModeChange if $should and currentState is $currentState',
'should not call processAnnotationModeChange when height is $height and width is $width and currentState is $currentState',
({ currentState, height, width }) => {
image.annotationControlsFSM = new AnnotationControlsFSM(currentState);

Expand Down