fix(discoverability): add reset logic to handleStop (#602)

ChenCodes · web-flow · commit d59c01213acc · 2020-09-28T23:54:51.000Z
diff --git a/src/region/RegionAnnotations.tsx b/src/region/RegionAnnotations.tsx
@@ -14,6 +14,7 @@ type Props = {
     isDiscoverabilityEnabled: boolean;
     isRotated: boolean;
     location: number;
+    resetCreator: () => void;
     setActiveAnnotationId: (annotationId: string | null) => void;
     setReferenceShape: (rect: DOMRect) => void;
     setStaged: (staged: CreatorItemRegion | null) => void;
@@ -45,6 +46,11 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
         }
     }
 
+    handleAbort = (): void => {
+        const { resetCreator } = this.props;
+        resetCreator();
+    };
+
     handleAnnotationActive = (annotationId: string | null): void => {
         const { setActiveAnnotationId } = this.props;
 
@@ -74,6 +80,7 @@ export default class RegionAnnotations extends React.PureComponent<Props, State>
                         className={classNames('ba-RegionAnnotations-creator', {
                             'is-discoverability-enabled': isDiscoverabilityEnabled,
                         })}
+                        onAbort={this.handleAbort}
                         onStart={this.handleStart}
                         onStop={this.handleStop}
                     />
diff --git a/src/region/RegionContainer.tsx b/src/region/RegionContainer.tsx
@@ -11,6 +11,7 @@ import {
     isCreatorStagedRegion,
     isFeatureEnabled,
     Mode,
+    resetCreatorAction,
     setActiveAnnotationIdAction,
     setReferenceShapeAction,
     setStagedAction,
@@ -43,6 +44,7 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe
 };
 
 export const mapDispatchToProps = {
+    resetCreator: resetCreatorAction,
     setActiveAnnotationId: setActiveAnnotationIdAction,
     setReferenceShape: setReferenceShapeAction,
     setStaged: setStagedAction,
diff --git a/src/region/RegionCreator.tsx b/src/region/RegionCreator.tsx
@@ -10,6 +10,7 @@ import './RegionCreator.scss';
 
 type Props = {
     className?: string;
+    onAbort: () => void;
     onStart: () => void;
     onStop: (shape: Rect) => void;
 };
@@ -18,7 +19,7 @@ const MIN_X = 0; // Minimum region x position must be positive
 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
 
-export default function RegionCreator({ className, onStart, onStop }: Props): JSX.Element {
+export default function RegionCreator({ className, onAbort, 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);
@@ -109,6 +110,8 @@ export default function RegionCreator({ className, onStart, onStop }: Props): JS
 
         if (shape) {
             onStop(shape);
+        } else {
+            onAbort();
         }
     };
     const updateDraw = (x: number, y: number): void => {
diff --git a/src/region/__tests__/RegionAnnotations-test.tsx b/src/region/__tests__/RegionAnnotations-test.tsx
@@ -18,6 +18,7 @@ describe('RegionAnnotations', () => {
     const defaults = {
         activeAnnotationId: null,
         location: 1,
+        resetCreator: jest.fn(),
         setActiveAnnotationId: jest.fn(),
         setMessage: jest.fn(),
         setReferenceShape: jest.fn(),
@@ -49,6 +50,14 @@ describe('RegionAnnotations', () => {
             instance = wrapper.instance() as InstanceType<typeof RegionAnnotations>;
         });
 
+        describe('handleAbort', () => {
+            test('should call resetCreator', () => {
+                instance.handleAbort();
+
+                expect(defaults.resetCreator).toHaveBeenCalled();
+            });
+        });
+
         describe('handleStart', () => {
             test('should reset the creator status', () => {
                 instance.handleStart();
@@ -160,6 +169,7 @@ describe('RegionAnnotations', () => {
                   />
                   <RegionCreator
                     className="ba-RegionAnnotations-creator"
+                    onAbort={[Function]}
                     onStart={[Function]}
                     onStop={[Function]}
                   />
@@ -173,6 +183,7 @@ describe('RegionAnnotations', () => {
                 <Fragment>
                   <RegionCreator
                     className="ba-RegionAnnotations-creator is-discoverability-enabled"
+                    onAbort={[Function]}
                     onStart={[Function]}
                     onStop={[Function]}
                   />
diff --git a/src/region/__tests__/RegionCreator-test.tsx b/src/region/__tests__/RegionCreator-test.tsx
@@ -13,6 +13,12 @@ jest.mock('../RegionRect');
 jest.mock('../regionUtil');
 
 describe('RegionCreator', () => {
+    const defaults = {
+        onAbort: jest.fn(),
+        onStart: jest.fn(),
+        onStop: jest.fn(),
+    };
+
     const getDOMRect = (x = 0, y = 0, height = 1000, width = 1000): DOMRect => ({
         bottom: y + height,
         top: y,
@@ -26,8 +32,7 @@ describe('RegionCreator', () => {
     });
 
     // Render helpers
-    const getWrapper = (props = {}): ReactWrapper =>
-        mount(<RegionCreator onStart={jest.fn()} onStop={jest.fn()} {...props} />);
+    const getWrapper = (props = {}): ReactWrapper => mount(<RegionCreator {...defaults} {...props} />);
     const getWrapperRoot = (wrapper: ReactWrapper): ReactWrapper => wrapper.find('[data-testid="ba-RegionCreator"]');
 
     beforeEach(() => {
@@ -90,18 +95,16 @@ describe('RegionCreator', () => {
         });
 
         test('should call the onStart and onStop callback when drawing starts and stops', () => {
-            const onStart = jest.fn();
-            const onStop = jest.fn();
-            const wrapper = getWrapper({ onStart, onStop });
+            const wrapper = getWrapper();
 
             simulateDrawStart(wrapper, 50, 50);
             simulateDrawMove(100, 100);
             simulateDrawStop();
             jest.advanceTimersByTime(1000);
             wrapper.update();
 
-            expect(onStart).toHaveBeenCalled();
-            expect(onStop).toHaveBeenCalledWith({
+            expect(defaults.onStart).toHaveBeenCalled();
+            expect(defaults.onStop).toHaveBeenCalledWith({
                 height: 5,
                 type: 'rect',
                 width: 5,
@@ -110,10 +113,20 @@ describe('RegionCreator', () => {
             });
         });
 
+        test('should call onStart and onAbort callback when user clicks without dragging', () => {
+            const wrapper = getWrapper();
+
+            simulateDrawStart(wrapper, 50, 50);
+            simulateDrawStop();
+            jest.advanceTimersByTime(1000);
+            wrapper.update();
+
+            expect(defaults.onStart).toHaveBeenCalled();
+            expect(defaults.onAbort).toHaveBeenCalled();
+        });
+
         test('should do nothing if primary button is not pressed', () => {
-            const onStart = jest.fn();
-            const onStop = jest.fn();
-            const wrapper = getWrapper({ onStart, onStop });
+            const wrapper = getWrapper();
 
             act(() => {
                 wrapper.simulate('mousedown', { buttons: 2, clientX: 50, clientY: 50 });
@@ -123,21 +136,19 @@ describe('RegionCreator', () => {
             jest.advanceTimersByTime(1000);
             wrapper.update();
 
-            expect(onStart).not.toHaveBeenCalled();
-            expect(onStop).not.toHaveBeenCalled();
+            expect(defaults.onStart).not.toHaveBeenCalled();
+            expect(defaults.onStop).not.toHaveBeenCalled();
         });
 
         test('should do nothing if the mouse is moved without being pressed over the wrapper first', () => {
-            const onStart = jest.fn();
-            const onStop = jest.fn();
-            const wrapper = getWrapper({ onStart, onStop });
+            const wrapper = getWrapper();
 
             simulateDrawMove(50, 50);
             jest.advanceTimersByTime(1000);
             wrapper.update();
 
-            expect(onStart).not.toHaveBeenCalled();
-            expect(onStop).not.toHaveBeenCalled();
+            expect(defaults.onStart).not.toHaveBeenCalled();
+            expect(defaults.onStop).not.toHaveBeenCalled();
         });
 
         test('should prevent click events from surfacing to the parent document', () => {
@@ -218,33 +229,29 @@ describe('RegionCreator', () => {
         });
 
         test('should call the onStart and onStop callback when drawing starts and stops', () => {
-            const onStart = jest.fn();
-            const onStop = jest.fn();
-            const wrapper = getWrapper({ onStart, onStop });
+            const wrapper = getWrapper();
 
             simulateDrawStart(wrapper, 50, 50);
             simulateDrawMove(wrapper, 100, 100);
             simulateDrawStop(wrapper);
             jest.advanceTimersByTime(1000);
             wrapper.update();
 
-            expect(onStart).toHaveBeenCalled();
-            expect(onStop).toHaveBeenCalledWith(shape);
+            expect(defaults.onStart).toHaveBeenCalled();
+            expect(defaults.onStop).toHaveBeenCalledWith(shape);
         });
 
         test('should call the onStart and onStop callback even if drawing is cancelled', () => {
-            const onStart = jest.fn();
-            const onStop = jest.fn();
-            const wrapper = getWrapper({ onStart, onStop });
+            const wrapper = getWrapper();
 
             simulateDrawStart(wrapper, 50, 50);
             simulateDrawMove(wrapper, 100, 100);
             simulateDrawCancel(wrapper);
             jest.advanceTimersByTime(1000);
             wrapper.update();
 
-            expect(onStart).toHaveBeenCalled();
-            expect(onStop).toHaveBeenCalledWith(shape);
+            expect(defaults.onStart).toHaveBeenCalled();
+            expect(defaults.onStop).toHaveBeenCalledWith(shape);
         });
     });
 
diff --git a/src/store/eventing/__tests__/staged-test.ts b/src/store/eventing/__tests__/staged-test.ts
@@ -93,17 +93,11 @@ describe('store/eventing/staged', () => {
     });
 
     describe('handleResetCreatorAction()', () => {
-        test('should not emit event if type is null', () => {
-            const prevState = createStore().getState();
-            handleResetCreatorAction(prevState);
-
-            expect(eventManager.emit).not.toHaveBeenCalled();
-        });
-
         test.each`
-            prev                      | type
-            ${getCreatorState()}      | ${'highlight'}
-            ${getCreatorState(false)} | ${'region'}
+            prev                        | type
+            ${createStore().getState()} | ${null}
+            ${getCreatorState()}        | ${'highlight'}
+            ${getCreatorState(false)}   | ${'region'}
         `('should emit cancel event if with type=$type', ({ prev, type }) => {
             handleResetCreatorAction(prev);
 
diff --git a/src/store/eventing/staged.ts b/src/store/eventing/staged.ts
@@ -52,9 +52,5 @@ export const handleResetCreatorAction = (prevState: AppState): void => {
     const prevStaged = getCreatorStaged(prevState);
     const type = getType(prevStaged);
 
-    if (!type) {
-        return;
-    }
-
     eventManager.emit(Event.CREATOR_STAGED_CHANGE, { type, status: 'cancel' });
 };
