feat(region): Retain annotation message between pages (#498)

* feat(region): Retain annotation message between pages * feat(region): Address comments Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
box · May 27, 2020 · f4553f7 · f4553f7
1 parent 92588cb
commit f4553f7
Show file tree

Hide file tree

Showing 14 changed files with 90 additions and 81 deletions.
diff --git a/src/region/RegionAnnotations.tsx b/src/region/RegionAnnotations.tsx
@@ -1,22 +1,24 @@
-import merge from 'lodash/merge';
 import * as React from 'react';
 import PopupReply from '../components/Popups/PopupReply';
 import RegionCreator from './RegionCreator';
 import RegionList from './RegionList';
 import RegionRect, { RegionRectRef } from './RegionRect';
 import { AnnotationRegion, Rect } from '../@types';
+import { CreateArg } from './actions';
 import { CreatorItem, CreatorStatus } from '../store/creator';
 import { scaleShape } from './regionUtil';
 import './RegionAnnotations.scss';
 
 type Props = {
     activeAnnotationId: string | null;
     annotations: AnnotationRegion[];
-    createRegion: (arg: { location: number; message: string; shape: Rect }) => void;
+    createRegion: (arg: CreateArg) => void;
     isCreating: boolean;
+    message: string;
     page: number;
     scale: number;
     setActiveAnnotationId: (annotationId: string | null) => void;
+    setMessage: (message: string) => void;
     setStaged: (staged: CreatorItem | null) => void;
     setStatus: (status: CreatorStatus) => void;
     staged?: CreatorItem | null;
@@ -36,11 +38,6 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
 
     state: State = {};
 
-    setStaged(data: Partial<CreatorItem> | null): void {
-        const { page, setStaged, staged } = this.props;
-        setStaged(data ? merge({ location: page, message: '' }, staged, data) : data);
-    }
-
     setStatus(status: CreatorStatus): void {
         const { setStatus } = this.props;
         setStatus(status);
@@ -53,44 +50,46 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
     };
 
     handleCancel = (): void => {
-        this.setStaged(null);
+        const { setMessage, setStaged } = this.props;
+        setMessage('');
+        setStaged(null);
         this.setStatus(CreatorStatus.init);
     };
 
     handleChange = (text?: string): void => {
-        this.setStaged({ message: text });
+        const { setMessage } = this.props;
+        setMessage(text || '');
     };
 
     handleStart = (): void => {
-        this.setStaged(null);
+        const { setStaged } = this.props;
+        setStaged(null);
         this.setStatus(CreatorStatus.init);
     };
 
     handleStop = (shape: Rect): void => {
-        const { scale } = this.props;
-
-        this.setStaged({ shape: scaleShape(shape, scale, true) });
+        const { page, scale, setStaged } = this.props;
+        setStaged({ location: page, shape: scaleShape(shape, scale, true) });
         this.setStatus(CreatorStatus.staged);
     };
 
     handleSubmit = (): void => {
-        const { createRegion, staged } = this.props;
+        const { createRegion, message, staged } = this.props;
 
         if (!staged) {
             return;
         }
 
-        createRegion(staged);
+        createRegion({ ...staged, message });
     };
 
     setRectRef = (rectRef: RegionRectRef): void => {
         this.setState({ rectRef });
     };
 
     render(): JSX.Element {
-        const { activeAnnotationId, annotations, isCreating, scale, staged, status } = this.props;
+        const { activeAnnotationId, annotations, isCreating, message, scale, staged, status } = this.props;
         const { rectRef } = this.state;
-        const canDraw = !staged || !staged.message;
         const canReply = status !== CreatorStatus.init;
         const isPending = status === CreatorStatus.pending;
 
@@ -108,7 +107,6 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
                 {/* Layer 2: Drawn (unsaved) incomplete annotation target, if any */}
                 {isCreating && (
                     <RegionCreator
-                        canDraw={canDraw}
                         className="ba-RegionAnnotations-creator"
                         onStart={this.handleStart}
                         onStop={this.handleStop}
@@ -131,7 +129,7 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
                             onChange={this.handleChange}
                             onSubmit={this.handleSubmit}
                             reference={rectRef}
-                            value={staged.message}
+                            value={message}
                         />
                     </div>
                 )}

diff --git a/src/region/RegionContainer.tsx b/src/region/RegionContainer.tsx
@@ -7,9 +7,11 @@ import {
     getActiveAnnotationId,
     getAnnotationMode,
     getAnnotationsForLocation,
+    getCreatorMessage,
     getCreatorStagedForLocation,
     getCreatorStatus,
     setActiveAnnotationIdAction,
+    setMessageAction,
     setStagedAction,
     setStatusAction,
 } from '../store';
@@ -22,6 +24,7 @@ export type Props = {
     activeAnnotationId: string | null;
     annotations: AnnotationRegion[];
     isCreating: boolean;
+    message: string;
     staged: CreatorItem | null;
     status: CreatorStatus;
 };
@@ -30,13 +33,15 @@ export const mapStateToProps = (state: AppState, { page }: { page: number }): Pr
     activeAnnotationId: getActiveAnnotationId(state),
     annotations: getAnnotationsForLocation(state, page).filter(isRegion),
     isCreating: getAnnotationMode(state) === 'region',
+    message: getCreatorMessage(state),
     staged: getCreatorStagedForLocation(state, page),
     status: getCreatorStatus(state),
 });
 
 export const mapDispatchToProps = {
     createRegion: createRegionAction,
     setActiveAnnotationId: setActiveAnnotationIdAction,
+    setMessage: setMessageAction,
     setStaged: setStagedAction,
     setStatus: setStatusAction,
 };

diff --git a/src/region/RegionCreator.scss b/src/region/RegionCreator.scss
@@ -2,7 +2,7 @@ $region_cursor_32: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAA
 $region_cursor_32_2x: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAYAAACqaXHeAAAABGdBTUEAALGPC/xhBQAAADhlWElmTU0AKgAAAAgAAYdpAAQAAAABAAAAGgAAAAAAAqACAAQAAAABAAAAQKADAAQAAAABAAAAQAAAAABlmWCKAAABWUlEQVR4Ae2YOxKDIBCGNaM5R4pYWOT+B0lhYQrPEZ1JGGcoZIy6Lj4IH40DLAt8+8srSUgQgAAEIAABCEAAAnESSM867Y9JmrGlJi1pf1li9M822dknd7/dREN8NY3IHgWIcB1oPBdZqVLsVFCAJRHK113dtbsFCtg78iZib9Nn7vTbmshenbJdsihgF8wjnZRF0ZdWdT1S+7tI+8+7nlGAS+Ss+bX7/Nx8UMAcoaPr506A2vGhAC3Brdq7J76t+oleAQDYSlqh+FW/CHk6meVSP77WiOh/gUUvp1NytpF7lGXSdd2U6aCubds+n+fuxXBgNshkWZY8q6ovQwEDNOsz3hQgHcLa26DtBwVYEsqveheQRsKsGbwIKYPmtXn02yAAvOopQGcoIMCgeR0yCvCKM0Bn0StAfReQBp2ToJQY9hCAAAQgAAEIQAACEIAABCAAAQhAwDOBLxaXQ2IYNsXoAAAAAElFTkSuQmCC';
 $region_cursor_32_3x: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGAAAABgCAYAAADimHc4AAAABGdBTUEAALGPC/xhBQAAADhlWElmTU0AKgAAAAgAAYdpAAQAAAABAAAAGgAAAAAAAqACAAQAAAABAAAAYKADAAQAAAABAAAAYAAAAACK+310AAACaUlEQVR4Ae3bwUrDQBDG8VTE+Bg9CD0IBd//IYSCh4KHPobxopuDYQgGOtNvdhvy39O2zc5uf9Odpol2HQ0BBBBAAAEEEEAAAQQQQAABBBBAAAEEEEAAgXSBXfoMdzDBT2mZy9iVFo3/EB3IOI0ACdA4hqM8hkeudODLfi9Z+eflIonDDpAwxoOQgLidZOTmSpBV85YRVfmya2AHWI0GfRLQAN1OuekSZCGWfkxl/4hjB9gsNOiTgAbodsrVlKBSCr7Kwnu7+Fl/KGXkefbc3T9kBzROEQlonIDVlCDr9Ho4dMMwdH3fdx/ns30p3M8+21laGDtgSabS8ySgEvTSNKssQUtvxvt8xrUd7xrYAV4x8fEkQAzqDbe5EuS9BO0F9R7PDvCKiY8nAWJQb7hNlKClS81erIzj2QEZqo6YJMCBlXEoCchQdcQkAQ6sjENJQIaqI2b6WVDFy7y9aq6aZ03sAMenNeNQEpCh6ogZ/seCa+ewZeHteOy+y52sW9t4N+yvjXfFbm1PJcb76TSFqVmC0r8DpndVOiO+xbOvRfvqeNF1RMdVTcD4SVM0i67aAYp1RWJULUGRBf43JuOmvJ2nZgniS9jKN+iTgAbodsr07wDVdi5nU/xpos0cfY0AJUjjGI5CAsJ0moEkQOMYjkICwnSagSRA4xiOQgLCdJqBJEDjGI5CAsJ0moEkQOMYjkICwnSagSRA4xiOQgLCdJqBJEDjGI6SfkcsvLLZQC5Hz0B4qBGgBGkciYIAAggggAACCCCAAAIIIIAAAggggAACCCCAAAIIIIAAAggggAACCCCAAAIITAK/BlNVeN572e8AAAAASUVORK5CYII=';
 
-.ba-RegionCreator.is-active {
+.ba-RegionCreator {
     cursor: url($region_cursor_32) 16 16, crosshair; /* Legacy */
     cursor: image-set(url($region_cursor_32) 1x, url($region_cursor_32_2x) 2x, url($region_cursor_32_3x) 3x) 16 16, crosshair; /* Webkit */ /* stylelint-disable-line */
 }

diff --git a/src/region/RegionCreator.tsx b/src/region/RegionCreator.tsx
@@ -8,7 +8,6 @@ import { styleShape } from './regionUtil';
 import './RegionCreator.scss';
 
 type Props = {
-    canDraw: boolean;
     className?: string;
     onStart: () => void;
     onStop: (shape: Rect) => void;
@@ -19,7 +18,7 @@ const MIN_Y = 0; // Minimum region y position must be positive
 const MIN_SIZE = 10; // Minimum region size must be large enough to be clickable
 const MOUSE_PRIMARY = 1; // Primary mouse button
 
-export default function RegionCreator({ canDraw, className, onStart, onStop }: Props): JSX.Element {
+export default function RegionCreator({ className, onStart, onStop }: Props): JSX.Element {
     const [isDrawing, setIsDrawing] = React.useState<boolean>(false);
     const [isHovering, setIsHovering] = React.useState<boolean>(false);
     const creatorElRef = React.useRef<HTMLDivElement>(null);
@@ -154,18 +153,6 @@ export default function RegionCreator({ canDraw, className, onStart, onStop }: P
     const handleTouchStart = ({ targetTouches }: React.TouchEvent): void => {
         startDraw(targetTouches[0].clientX, targetTouches[0].clientY);
     };
-    const eventHandlers = canDraw
-        ? {
-              onClick: handleClick,
-              onMouseDown: handleMouseDown,
-              onMouseOut: handleMouseOut,
-              onMouseOver: handleMouseOver,
-              onTouchCancel: handleTouchCancel,
-              onTouchEnd: handleTouchEnd,
-              onTouchMove: handleTouchMove,
-              onTouchStart: handleTouchStart,
-          }
-        : {};
 
     const renderStep = (callback: () => void): void => {
         renderHandleRef.current = window.requestAnimationFrame(callback);
@@ -216,14 +203,23 @@ export default function RegionCreator({ canDraw, className, onStart, onStop }: P
     });
 
     return (
+        // eslint-disable-next-line jsx-a11y/mouse-events-have-key-events
         <div
             ref={creatorElRef}
-            className={classNames(className, 'ba-RegionCreator', { 'is-active': canDraw })}
+            className={classNames(className, 'ba-RegionCreator')}
             data-testid="ba-RegionCreator"
-            {...eventHandlers}
+            onClick={handleClick}
+            onMouseDown={handleMouseDown}
+            onMouseOut={handleMouseOut}
+            onMouseOver={handleMouseOver}
+            onTouchCancel={handleTouchCancel}
+            onTouchEnd={handleTouchEnd}
+            onTouchMove={handleTouchMove}
+            onTouchStart={handleTouchStart}
+            role="presentation"
         >
             {isDrawing && <RegionRect ref={regionRectRef} className="ba-RegionCreator-rect" isActive />}
-            {!isDrawing && isHovering && canDraw && <PopupCursor />}
+            {!isDrawing && isHovering && <PopupCursor />}
         </div>
     );
 }
diff --git a/src/region/__tests__/RegionAnnotations-test.tsx b/src/region/__tests__/RegionAnnotations-test.tsx
@@ -18,13 +18,15 @@ jest.mock('../regionUtil', () => ({
     scaleShape: jest.fn(value => value),
 }));
 
-describe('RegionAnnotations', () => {
+describe('components/region/RegionAnnotations', () => {
     const defaults = {
         activeAnnotationId: null,
         createRegion: jest.fn(),
+        message: 'test',
         page: 1,
         scale: 1,
         setActiveAnnotationId: jest.fn(),
+        setMessage: jest.fn(),
         setStaged: jest.fn(),
         setStatus: jest.fn(),
         staged: null,
@@ -40,7 +42,6 @@ describe('RegionAnnotations', () => {
     const getRectRef = (): HTMLDivElement => document.createElement('div');
     const getStaged = (): CreatorItem => ({
         location: 1,
-        message: 'test',
         shape: getRect(),
     });
     const getWrapper = (props = {}): ShallowWrapper => shallow(<RegionAnnotations {...defaults} {...props} />);
@@ -60,6 +61,7 @@ describe('RegionAnnotations', () => {
             test('should reset the staged state and status', () => {
                 instance.handleCancel();
 
+                expect(defaults.setMessage).toHaveBeenCalledWith('');
                 expect(defaults.setStaged).toHaveBeenCalledWith(null);
                 expect(defaults.setStatus).toHaveBeenCalledWith(CreatorStatus.init);
             });
@@ -69,18 +71,15 @@ describe('RegionAnnotations', () => {
             test('should set the staged state with the new message', () => {
                 instance.handleChange('test');
 
-                expect(defaults.setStaged).toHaveBeenCalledWith(
-                    expect.objectContaining({
-                        message: 'test',
-                    }),
-                );
+                expect(defaults.setMessage).toHaveBeenCalledWith('test');
             });
         });
 
         describe('handleStart', () => {
             test('should reset the creator status', () => {
                 instance.handleStart();
 
+                expect(defaults.setStaged).toHaveBeenCalledWith(null);
                 expect(defaults.setStatus).toHaveBeenCalledWith(CreatorStatus.init);
             });
         });
@@ -98,7 +97,10 @@ describe('RegionAnnotations', () => {
                 instance.handleStop(shape);
 
                 expect(scaleShape).toHaveBeenCalledWith(shape, defaults.scale, true); // Inverse scale
-                expect(defaults.setStaged).toHaveBeenCalledWith(expect.objectContaining({ shape }));
+                expect(defaults.setStaged).toHaveBeenCalledWith({
+                    location: defaults.page,
+                    shape,
+                });
                 expect(defaults.setStatus).toHaveBeenCalledWith(CreatorStatus.staged);
             });
         });
@@ -107,7 +109,10 @@ describe('RegionAnnotations', () => {
             test('should save the staged annotation', () => {
                 instance.handleSubmit();
 
-                expect(defaults.createRegion).toHaveBeenCalledWith(getStaged());
+                expect(defaults.createRegion).toHaveBeenCalledWith({
+                    ...getStaged(),
+                    message: defaults.message,
+                });
             });
         });
 
@@ -134,7 +139,6 @@ describe('RegionAnnotations', () => {
             const creator = wrapper.find(RegionCreator);
 
             expect(creator.hasClass('ba-RegionAnnotations-creator')).toBe(true);
-            expect(creator.prop('canDraw')).toBe(true);
         });
 
         test('should scale and render a RegionAnnotation if one has been drawn', () => {

diff --git a/src/region/__tests__/RegionContainer-test.tsx b/src/region/__tests__/RegionContainer-test.tsx
@@ -27,11 +27,13 @@ describe('RegionContainer', () => {
                 annotations: [],
                 createRegion: expect.any(Function),
                 isCreating: false,
+                message: '',
                 page: defaults.page,
                 scale: defaults.scale,
                 staged: null,
                 status: CreatorStatus.init,
                 setActiveAnnotationId: expect.any(Function),
+                setMessage: expect.any(Function),
                 setStaged: expect.any(Function),
                 setStatus: expect.any(Function),
                 store: defaults.store,

diff --git a/src/region/__tests__/RegionCreator-test.tsx b/src/region/__tests__/RegionCreator-test.tsx
@@ -13,7 +13,6 @@ jest.mock('../regionUtil');
 
 describe('RegionCreator', () => {
     const defaults = {
-        canDraw: true,
         onDraw: jest.fn(),
         onStart: jest.fn(),
         onStop: jest.fn(),
@@ -278,36 +277,21 @@ describe('RegionCreator', () => {
             });
         });
 
-        test('should add event listeners if canDraw is true', () => {
+        test('should add class and event listeners', () => {
             const wrapper = getWrapper();
             const rootEl = getWrapperRoot(wrapper);
 
+            expect(rootEl.hasClass('ba-RegionCreator')).toBe(true);
+
             expect(rootEl.prop('onClick')).toBeDefined();
             expect(rootEl.prop('onMouseDown')).toBeDefined();
+            expect(rootEl.prop('onMouseOut')).toBeDefined();
+            expect(rootEl.prop('onMouseOver')).toBeDefined();
+            expect(rootEl.prop('onTouchCancel')).toBeDefined();
             expect(rootEl.prop('onTouchCancel')).toBeDefined();
             expect(rootEl.prop('onTouchEnd')).toBeDefined();
             expect(rootEl.prop('onTouchMove')).toBeDefined();
             expect(rootEl.prop('onTouchStart')).toBeDefined();
         });
-
-        test('should not add event listeners if canDraw is false', () => {
-            const wrapper = getWrapper({ canDraw: false });
-            const rootEl = getWrapperRoot(wrapper);
-
-            expect(rootEl.prop('onClick')).not.toBeDefined();
-            expect(rootEl.prop('onMouseDown')).not.toBeDefined();
-            expect(rootEl.prop('onTouchCancel')).not.toBeDefined();
-            expect(rootEl.prop('onTouchEnd')).not.toBeDefined();
-            expect(rootEl.prop('onTouchMove')).not.toBeDefined();
-            expect(rootEl.prop('onTouchStart')).not.toBeDefined();
-        });
-
-        test.each([true, false])('should add the is-active class based on canDraw: %s', canDraw => {
-            const wrapper = getWrapper({ canDraw });
-            const rootEl = getWrapperRoot(wrapper);
-
-            expect(rootEl.hasClass('ba-RegionCreator')).toBe(true);
-            expect(rootEl.hasClass('is-active')).toBe(canDraw);
-        });
     });
 });
diff --git a/src/store/creator/__mocks__/creatorState.ts b/src/store/creator/__mocks__/creatorState.ts
@@ -3,9 +3,9 @@ import { CreatorStatus } from '../types';
 export default {
     cursor: 0,
     error: null,
+    message: 'test',
     staged: {
         location: 1,
-        message: 'test',
         shape: {
             height: 100,
             width: 100,