fix(popup): Use reference id instead of shape (#629)

box · Nov 2, 2020 · 2a0e8c6 · 2a0e8c6
1 parent db5e289
commit 2a0e8c6
Show file tree

Hide file tree

Showing 28 changed files with 170 additions and 285 deletions.
diff --git a/package.json b/package.json
@@ -21,6 +21,7 @@
         "@popperjs/core": "^2.4.0",
         "@reduxjs/toolkit": "^1.3.5",
         "@types/react-redux": "^7.1.7",
+        "@types/uuid": "^8.3.0",
         "axios": "^0.19.2",
         "box-ui-elements": "^12.0.0-beta.146",
         "classnames": "^2.2.5",
@@ -34,7 +35,8 @@
         "react-tether": "1.0.5",
         "react-textarea-autosize": "^7.1.2",
         "redux": "^4.0.5",
-        "scroll-into-view-if-needed": "^2.2.24"
+        "scroll-into-view-if-needed": "^2.2.24",
+        "uuid": "^8.3.1"
     },
     "devDependencies": {
         "@babel/cli": "^7.8.4",

diff --git a/src/common/__tests__/useWindowSize-test.tsx b/src/common/__tests__/useWindowSize-test.tsx
diff --git a/src/common/useWindowSize.ts b/src/common/useWindowSize.ts
diff --git a/src/highlight/HighlightAnnotations.tsx b/src/highlight/HighlightAnnotations.tsx
@@ -5,7 +5,6 @@ import HighlightSvg from './HighlightSvg';
 import HighlightTarget from './HighlightTarget';
 import PopupHighlight from '../components/Popups/PopupHighlight';
 import PopupHighlightError from '../components/Popups/PopupHighlightError';
-import useWindowSize from '../common/useWindowSize';
 import { AnnotationHighlight } from '../@types';
 import { CreateArg } from './actions';
 import { CreatorItemHighlight, CreatorStatus, SelectionArg, SelectionItem } from '../store';
@@ -23,7 +22,7 @@ type Props = {
     selection: SelectionItem | null;
     setActiveAnnotationId: (annotationId: string | null) => void;
     setIsPromoting: (isPromoting: boolean) => void;
-    setReferenceShape: (rect: DOMRect) => void;
+    setReferenceId: (uuid: string) => void;
     setSelection: (selection: SelectionArg | null) => void;
     setStaged: (staged: CreatorItemHighlight | null) => void;
     setStatus: (status: CreatorStatus) => void;
@@ -40,15 +39,12 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
         selection,
         setActiveAnnotationId,
         setIsPromoting,
-        setReferenceShape,
+        setReferenceId,
         setSelection,
         setStaged,
         setStatus,
         staged,
     } = props;
-    const [highlightRef, setHighlightRef] = React.useState<HTMLAnchorElement | null>(null);
-    const windowSize = useWindowSize();
-
     const canCreate = isCreating || isPromoting;
 
     const handleAnnotationActive = (annotationId: string | null): void => {
@@ -82,6 +78,10 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
         setSelection(null);
     };
 
+    const handleStagedMount = (uuid: string): void => {
+        setReferenceId(uuid);
+    };
+
     React.useEffect(() => {
         if (!isSelecting) {
             return;
@@ -99,14 +99,6 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
         stageSelection();
     }, [isCreating, selection]); // eslint-disable-line react-hooks/exhaustive-deps
 
-    React.useEffect(() => {
-        if (highlightRef === null) {
-            return;
-        }
-
-        setReferenceShape(highlightRef.getBoundingClientRect());
-    }, [highlightRef, windowSize]); // eslint-disable-line react-hooks/exhaustive-deps
-
     return (
         <>
             {/* Layer 1: Saved annotations */}
@@ -117,7 +109,12 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
                 <div className="ba-HighlightAnnotations-target">
                     <HighlightCanvas shapes={staged.shapes} />
                     <HighlightSvg>
-                        <HighlightTarget ref={setHighlightRef} annotationId="staged" isActive shapes={staged.shapes} />
+                        <HighlightTarget
+                            annotationId="staged"
+                            isActive
+                            onMount={handleStagedMount}
+                            shapes={staged.shapes}
+                        />
                     </HighlightSvg>
                 </div>
             )}

diff --git a/src/highlight/HighlightContainer.tsx b/src/highlight/HighlightContainer.tsx
@@ -17,7 +17,7 @@ import {
     SelectionItem,
     setActiveAnnotationIdAction,
     setIsPromotingAction,
-    setReferenceShapeAction,
+    setReferenceIdAction,
     setSelectionAction,
     setStagedAction,
     setStatusAction,
@@ -51,7 +51,7 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe
 export const mapDispatchToProps = {
     setActiveAnnotationId: setActiveAnnotationIdAction,
     setIsPromoting: setIsPromotingAction,
-    setReferenceShape: setReferenceShapeAction,
+    setReferenceId: setReferenceIdAction,
     setSelection: setSelectionAction,
     setStaged: setStagedAction,
     setStatus: setStatusAction,

diff --git a/src/highlight/HighlightTarget.tsx b/src/highlight/HighlightTarget.tsx
@@ -1,5 +1,6 @@
-import * as React from 'react';
+import React from 'react';
 import * as ReactRedux from 'react-redux';
+import * as uuid from 'uuid';
 import classNames from 'classnames';
 import noop from 'lodash/noop';
 import { getIsCurrentFileVersion } from '../store';
@@ -12,14 +13,16 @@ type Props = {
     className?: string;
     isActive?: boolean;
     onHover?: (annotationId: string | null) => void;
+    onMount?: (uuid: string) => void;
     onSelect?: (annotationId: string) => void;
     shapes: Array<Rect>;
 };
 
 export type HighlightTargetRef = HTMLAnchorElement;
 
 const HighlightTarget = (props: Props, ref: React.Ref<HighlightTargetRef>): JSX.Element => {
-    const { annotationId, className, isActive, onHover = noop, onSelect = noop, shapes } = props;
+    const { annotationId, className, isActive, onHover = noop, onMount = noop, onSelect = noop, shapes } = props;
+    const uuidRef = React.useRef<string>(uuid.v4());
     const isCurrentFileVersion = ReactRedux.useSelector(getIsCurrentFileVersion);
 
     const handleClick = (event: React.MouseEvent<HighlightTargetRef>): void => {
@@ -61,11 +64,16 @@ const HighlightTarget = (props: Props, ref: React.Ref<HighlightTargetRef>): JSX.
         event.currentTarget.focus();
     };
 
+    React.useEffect(() => {
+        onMount(uuidRef.current);
+    }, [onMount]);
+
     return (
         // eslint-disable-next-line jsx-a11y/anchor-is-valid
         <a
             ref={ref}
             className={classNames('ba-HighlightTarget', className, { 'is-active': isActive })}
+            data-ba-reference-id={uuidRef.current}
             data-resin-iscurrent={isCurrentFileVersion}
             data-resin-itemid={annotationId}
             data-resin-target="highlightText"

diff --git a/src/highlight/__tests__/HighlightAnnotations-test.tsx b/src/highlight/__tests__/HighlightAnnotations-test.tsx
@@ -28,7 +28,7 @@ describe('HighlightAnnotations', () => {
         selection: null,
         setActiveAnnotationId: jest.fn(),
         setIsPromoting: jest.fn(),
-        setReferenceShape: jest.fn(),
+        setReferenceId: jest.fn(),
         setSelection: jest.fn(),
         setStaged: jest.fn(),
         setStatus: jest.fn(),

diff --git a/src/highlight/__tests__/HighlightContainer-test.tsx b/src/highlight/__tests__/HighlightContainer-test.tsx
@@ -42,7 +42,7 @@ describe('HighlightContainer', () => {
                 selection: null,
                 setActiveAnnotationId: expect.any(Function),
                 setIsPromoting: expect.any(Function),
-                setReferenceShape: expect.any(Function),
+                setReferenceId: expect.any(Function),
                 setStaged: expect.any(Function),
                 setStatus: expect.any(Function),
                 staged: null,

diff --git a/src/highlight/__tests__/HighlightTarget-test.tsx b/src/highlight/__tests__/HighlightTarget-test.tsx
@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { MutableRefObject } from 'react';
 import * as ReactRedux from 'react-redux';
 import { shallow, ShallowWrapper } from 'enzyme';
 import HighlightTarget from '../HighlightTarget';
@@ -23,12 +23,14 @@ describe('HighlightTarget', () => {
             },
         ],
     };
-    const mockSetIsHover = jest.fn();
+
     const getWrapper = (props = {}): ShallowWrapper => shallow(<HighlightTarget {...defaults} {...props} />);
+    const mockInitalRef = { current: '123' } as MutableRefObject<string>;
 
     beforeEach(() => {
+        jest.spyOn(React, 'useEffect').mockImplementation(f => f());
+        jest.spyOn(React, 'useRef').mockImplementation(() => mockInitalRef);
         jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => true);
-        jest.spyOn(React, 'useState').mockImplementation(() => [false, mockSetIsHover]);
     });
 
     describe('render()', () => {
@@ -43,9 +45,10 @@ describe('HighlightTarget', () => {
 
         test.each([true, false])('should render classNames correctly when isActive is %s', isActive => {
             const wrapper = getWrapper({ isActive });
+            const anchor = wrapper.find('a');
 
-            expect(wrapper.hasClass('ba-HighlightTarget')).toBe(true);
-            expect(wrapper.hasClass('is-active')).toBe(isActive);
+            expect(anchor.hasClass('ba-HighlightTarget')).toBe(true);
+            expect(anchor.hasClass('is-active')).toBe(isActive);
         });
     });
 
@@ -88,12 +91,8 @@ describe('HighlightTarget', () => {
             test('should call onSelect', () => {
                 const wrapper = getWrapper();
                 const anchor = wrapper.find('a');
-                const event = {
-                    ...mockEvent,
-                    buttons: 1,
-                };
 
-                anchor.simulate('mousedown', event);
+                anchor.prop('onMouseDown')!((mockEvent as unknown) as React.MouseEvent);
 
                 expect(defaults.onSelect).toHaveBeenCalledWith('123');
                 expect(mockEvent.preventDefault).toHaveBeenCalled();
@@ -140,4 +139,13 @@ describe('HighlightTarget', () => {
             });
         });
     });
+
+    describe('onMount()', () => {
+        test('should call onMount with a generated uuid', () => {
+            const handleMount = jest.fn();
+            getWrapper({ onMount: handleMount });
+
+            expect(handleMount).toHaveBeenCalledWith(expect.any(String));
+        });
+    });
 });
diff --git a/src/popup/PopupContainer.tsx b/src/popup/PopupContainer.tsx
@@ -13,17 +13,16 @@ import {
     Mode,
     resetCreatorAction,
     setMessageAction,
-    getCreatorReferenceShape,
+    getCreatorReferenceId,
 } from '../store';
 import { createHighlightAction } from '../highlight/actions';
-import { Shape } from '../@types';
 import { createRegionAction } from '../region';
 
 export type Props = {
     isPromoting: boolean;
     message: string;
     mode: Mode;
-    popupReference?: Shape;
+    referenceId: string | null;
     staged: CreatorItem | null;
     status: CreatorStatus;
 };
@@ -33,7 +32,7 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe
         isPromoting: getIsPromoting(state),
         message: getCreatorMessage(state),
         mode: getAnnotationMode(state),
-        popupReference: getCreatorReferenceShape(state),
+        referenceId: getCreatorReferenceId(state),
         staged: getCreatorStagedForLocation(state, location),
         status: getCreatorStatus(state),
     };

diff --git a/src/popup/PopupLayer.tsx b/src/popup/PopupLayer.tsx
@@ -5,7 +5,6 @@ import { CreateArg as HighlightCreateArg } from '../highlight/actions';
 import { CreateArg as RegionCreateArg } from '../region/actions';
 import { CreatorItem, CreatorStatus, isCreatorStagedHighlight, isCreatorStagedRegion, Mode } from '../store';
 import { PopupReference } from '../components/Popups/Popper';
-import { Shape } from '../@types';
 import './PopupLayer.scss';
 
 type Props = {
@@ -15,7 +14,7 @@ type Props = {
     location: number;
     message: string;
     mode: Mode;
-    popupReference?: Shape;
+    referenceId: string | null;
     resetCreator: () => void;
     setMessage: (message: string) => void;
     staged?: CreatorItem | null;
@@ -34,7 +33,7 @@ const PopupLayer = (props: Props): JSX.Element | null => {
         isPromoting = false,
         message,
         mode,
-        popupReference,
+        referenceId,
         resetCreator,
         setMessage,
         staged,
@@ -67,25 +66,8 @@ const PopupLayer = (props: Props): JSX.Element | null => {
     };
 
     React.useEffect(() => {
-        if (!popupReference) {
-            return;
-        }
-
-        const { height, width, x, y } = popupReference;
-
-        const virtualElement = {
-            getBoundingClientRect: () => ({
-                bottom: y + height,
-                height,
-                left: x,
-                right: x + width,
-                top: y,
-                width,
-            }),
-        };
-
-        setReference(virtualElement);
-    }, [popupReference]);
+        setReference(referenceId ? document.querySelector(`[data-ba-reference-id="${referenceId}"]`) : null);
+    }, [referenceId]);
 
     return (
         <>