From 7ee83d987fba318805d78d9e769ef60faecc7440 Mon Sep 17 00:00:00 2001 From: Mingze Xiao Date: Tue, 8 Dec 2020 14:22:59 -0800 Subject: [PATCH 1/2] fix(highlight): Prevent changing staged during creator pending --- src/highlight/HighlightAnnotations.tsx | 6 ++++-- src/highlight/HighlightContainer.tsx | 4 ++++ .../__tests__/HighlightAnnotations-test.tsx | 13 +++++++++++++ .../__tests__/HighlightContainer-test.tsx | 14 +++++++++++++- 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/highlight/HighlightAnnotations.tsx b/src/highlight/HighlightAnnotations.tsx index ba72fdd5b..6e5c213e8 100644 --- a/src/highlight/HighlightAnnotations.tsx +++ b/src/highlight/HighlightAnnotations.tsx @@ -16,6 +16,7 @@ type Props = { annotations: AnnotationHighlight[]; createHighlight?: (arg: CreateArg) => void; isCreating: boolean; + isPending: boolean; isPromoting: boolean; isSelecting: boolean; location: number; @@ -34,6 +35,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { activeAnnotationId, annotations = [], isCreating = false, + isPending = false, isPromoting = false, isSelecting = false, selection, @@ -83,7 +85,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { }; React.useEffect(() => { - if (!isSelecting) { + if (!isSelecting || isPending) { return; } @@ -92,7 +94,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { }, [isSelecting]); // eslint-disable-line react-hooks/exhaustive-deps React.useEffect(() => { - if (!isCreating || !selection || selection.hasError) { + if (!isCreating || !selection || selection.hasError || isPending) { return; } diff --git a/src/highlight/HighlightContainer.tsx b/src/highlight/HighlightContainer.tsx index 8d0f4146e..b6d8b1f1d 100644 --- a/src/highlight/HighlightContainer.tsx +++ b/src/highlight/HighlightContainer.tsx @@ -5,10 +5,12 @@ import { AnnotationHighlight } from '../@types'; import { AppState, CreatorItemHighlight, + CreatorStatus, getActiveAnnotationId, getAnnotationMode, getAnnotationsForLocation, getCreatorStagedForLocation, + getCreatorStatus, getIsPromoting, getIsSelecting, getSelectionForLocation, @@ -28,6 +30,7 @@ export type Props = { activeAnnotationId: string | null; annotations: AnnotationHighlight[]; isCreating: boolean; + isPending: boolean; isPromoting: boolean; isSelecting: boolean; selection: SelectionItem | null; @@ -41,6 +44,7 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe activeAnnotationId: getActiveAnnotationId(state), annotations: getAnnotationsForLocation(state, location).filter(isHighlight), isCreating: getAnnotationMode(state) === Mode.HIGHLIGHT, + isPending: getCreatorStatus(state) === CreatorStatus.pending, isPromoting: getIsPromoting(state), isSelecting: getIsSelecting(state), selection: getSelectionForLocation(state, location), diff --git a/src/highlight/__tests__/HighlightAnnotations-test.tsx b/src/highlight/__tests__/HighlightAnnotations-test.tsx index 58495d1e6..97b1e9d42 100644 --- a/src/highlight/__tests__/HighlightAnnotations-test.tsx +++ b/src/highlight/__tests__/HighlightAnnotations-test.tsx @@ -22,6 +22,7 @@ describe('HighlightAnnotations', () => { activeAnnotationId: null, annotations: [], isCreating: false, + isPending: false, isPromoting: false, isSelecting: false, location: 1, @@ -176,6 +177,12 @@ describe('HighlightAnnotations', () => { expect(defaults.setStaged).not.toHaveBeenCalled(); expect(defaults.setStatus).not.toHaveBeenCalled(); }); + + test('should not reset staged and status if isPending is true', () => { + getWrapper({ isSelecting: true, isPending: true }); + expect(defaults.setStaged).not.toHaveBeenCalled(); + expect(defaults.setStatus).not.toHaveBeenCalled(); + }); }); describe('Creating a highlight', () => { @@ -193,6 +200,12 @@ describe('HighlightAnnotations', () => { }, ); + test('should not call setStaged and setStatus if isPending is true', () => { + getWrapper({ isCreating: true, selection: {}, isPending: true }); + expect(defaults.setStaged).not.toHaveBeenCalled(); + expect(defaults.setStatus).not.toHaveBeenCalled(); + }); + test('should call setStaged and setStatus if isCreating=true and selection is not null', () => { getWrapper({ isCreating: true, selection: selectionMock }); expect(defaults.setStaged).toHaveBeenCalledWith({ diff --git a/src/highlight/__tests__/HighlightContainer-test.tsx b/src/highlight/__tests__/HighlightContainer-test.tsx index 8dc96cbe3..2502b7f38 100644 --- a/src/highlight/__tests__/HighlightContainer-test.tsx +++ b/src/highlight/__tests__/HighlightContainer-test.tsx @@ -3,7 +3,7 @@ import { IntlShape } from 'react-intl'; import { ReactWrapper, mount } from 'enzyme'; import HighlightAnnotations from '../HighlightAnnotations'; import HighlightContainer, { Props } from '../HighlightContainer'; -import { createStore, CreatorItemHighlight, CreatorItemRegion, Mode } from '../../store'; +import { createStore, CreatorItemHighlight, CreatorItemRegion, CreatorStatus, Mode } from '../../store'; import { rect as highlightRect } from '../__mocks__/data'; import { rect as regionRect } from '../../region/__mocks__/data'; @@ -62,6 +62,18 @@ describe('HighlightContainer', () => { expect(wrapper.find(HighlightAnnotations).prop('isCreating')).toEqual(isCreating); }); + test.each` + status | isPending + ${CreatorStatus.init} | ${false} + ${CreatorStatus.pending} | ${true} + ${CreatorStatus.staged} | ${false} + `('should pass down isPending as $isPending if status is $status', ({ isPending, status }) => { + const store = createStore({ creator: { status } }); + const wrapper = getWrapper({ store }); + + expect(wrapper.find(HighlightAnnotations).prop('isPending')).toEqual(isPending); + }); + test.each` staged | expectedStaged ${stagedHighlight} | ${stagedHighlight} From 69c722d1350b6b05baa1e261af539f151109f983 Mon Sep 17 00:00:00 2001 From: Mingze Xiao Date: Tue, 8 Dec 2020 15:07:57 -0800 Subject: [PATCH 2/2] fix(highlight): Remove eslint disable line --- src/highlight/HighlightAnnotations.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/highlight/HighlightAnnotations.tsx b/src/highlight/HighlightAnnotations.tsx index 6e5c213e8..d619430da 100644 --- a/src/highlight/HighlightAnnotations.tsx +++ b/src/highlight/HighlightAnnotations.tsx @@ -53,7 +53,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { setActiveAnnotationId(annotationId); }; - const stageSelection = (): void => { + const stageSelection = React.useCallback((): void => { if (!selection) { return; } @@ -68,7 +68,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { })), }); setStatus(CreatorStatus.staged); - }; + }, [selection, setStaged, setStatus]); const handlePromote = (): void => { stageSelection(); @@ -91,7 +91,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { setStaged(null); setStatus(CreatorStatus.init); - }, [isSelecting]); // eslint-disable-line react-hooks/exhaustive-deps + }, [isSelecting, isPending, setStaged, setStatus]); React.useEffect(() => { if (!isCreating || !selection || selection.hasError || isPending) { @@ -99,7 +99,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => { } stageSelection(); - }, [isCreating, selection]); // eslint-disable-line react-hooks/exhaustive-deps + }, [isCreating, isPending, selection, stageSelection]); return ( <>