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.

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

fix(highlight): Remove selection when clicking anywhere outside of popup #615

Merged
merged 4 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 5 additions & 0 deletions src/common/__mocks__/useOutsideEvent.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default jest.fn((_type: string, _ref?: React.RefObject<Element>, callback?: () => void): void => {
if (callback) {
callback();
}
});
2 changes: 1 addition & 1 deletion src/common/useOutsideEvent.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';

// Utilizes the useEffect hook to handle when an event is fired on the document, but outside of the specified element
export default function useOutsideEvent(type: string, ref: React.RefObject<Element>, callback: () => void): void {
export default function useOutsideEvent(type: string, ref?: React.RefObject<Element>, callback?: () => void): void {
React.useEffect(() => {
function handleEvent(event: Event): void {
const containsEventTarget = !!ref?.current?.contains(event.target as Node);
Expand Down
6 changes: 5 additions & 1 deletion src/components/Popups/PopupHighlight.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import noop from 'lodash/noop';
import { FormattedMessage } from 'react-intl';
import messages from './messages';
import PopupBase from './PopupBase';
import useOutsideEvent from '../../common/useOutsideEvent';
import { Options } from './Popper';
import { Shape } from '../../@types/model';
import './PopupHighlight.scss';

export type Props = {
onCancel?: () => void;
onClick?: (event: React.MouseEvent) => void;
shape: Shape;
};
Expand Down Expand Up @@ -43,7 +45,7 @@ const options: Partial<Options> = {
placement: 'bottom',
};

export default function PopupHighlight({ onClick = noop, shape }: Props): JSX.Element {
export default function PopupHighlight({ onCancel = noop, onClick = noop, shape }: Props): JSX.Element {
const buttonRef = React.useRef<HTMLButtonElement>(null);
const { height, width, x, y } = shape;

Expand All @@ -62,6 +64,8 @@ export default function PopupHighlight({ onClick = noop, shape }: Props): JSX.El
onClick(event);
};

useOutsideEvent('mousedown', buttonRef, onCancel);

return (
<PopupBase className="ba-PopupHighlight" options={options} reference={reference}>
<button
Expand Down
10 changes: 8 additions & 2 deletions src/components/Popups/PopupHighlightError.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import React from 'react';
import noop from 'lodash/noop';
import { FormattedMessage } from 'react-intl';
import messages from './messages';
import PopupBase from './PopupBase';
import useOutsideEvent from '../../common/useOutsideEvent';
import { Options } from './Popper';
import { Shape } from '../../@types/model';
import './PopupHighlightError.scss';

export type Props = {
onCancel?: () => void;
shape: Shape;
};

Expand Down Expand Up @@ -40,7 +43,8 @@ const options: Partial<Options> = {
placement: 'bottom',
};

export default function PopupHighlightError({ shape }: Props): JSX.Element {
export default function PopupHighlightError({ onCancel = noop, shape }: Props): JSX.Element {
const popupRef = React.useRef<PopupBase>(null);
const { height, width, x, y } = shape;

const reference = {
Expand All @@ -54,8 +58,10 @@ export default function PopupHighlightError({ shape }: Props): JSX.Element {
}),
};

useOutsideEvent('mousedown', popupRef.current?.popupRef, onCancel);

return (
<PopupBase className="ba-PopupHighlightError" options={options} reference={reference}>
<PopupBase ref={popupRef} className="ba-PopupHighlightError" options={options} reference={reference}>
<FormattedMessage {...messages.popupHighlightRestrictedPrompt} />
</PopupBase>
);
Expand Down
22 changes: 15 additions & 7 deletions src/components/Popups/__tests__/PopupHighlight-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ import React from 'react';
import { shallow, ShallowWrapper } from 'enzyme';
import PopupBase from '../PopupBase';
import PopupHighlight from '../PopupHighlight';
import useOutsideEvent from '../../../common/useOutsideEvent';

jest.mock('../../../common/useOutsideEvent');

describe('PopupHighlight', () => {
const defaults = {
onCancel: jest.fn(),
onClick: jest.fn(),
shape: {
height: 10,
Expand All @@ -16,15 +20,19 @@ describe('PopupHighlight', () => {

const getWrapper = (props = {}): ShallowWrapper => shallow(<PopupHighlight {...defaults} {...props} />);

const buttonEl = {
addEventListener: jest.fn(),
removeEventListener: jest.fn(),
};
const buttonRef = React.createRef();

beforeEach(() => {
jest.spyOn(React, 'useRef').mockImplementationOnce(() => ({
current: buttonEl,
}));
jest.spyOn(React, 'useRef').mockImplementationOnce(() => buttonRef);
});

describe('event handlers', () => {
test('should call onCancel', () => {
getWrapper();

expect(defaults.onCancel).toHaveBeenCalled();
expect(useOutsideEvent).toHaveBeenCalledWith('mousedown', buttonRef, defaults.onCancel);
});
});

describe('mouse events', () => {
Expand Down
23 changes: 23 additions & 0 deletions src/components/Popups/__tests__/PopupHighlightError-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ import { FormattedMessage } from 'react-intl';
import { shallow, ShallowWrapper } from 'enzyme';
import PopupBase from '../PopupBase';
import PopupHighlightError from '../PopupHighlightError';
import useOutsideEvent from '../../../common/useOutsideEvent';

jest.mock('../../../common/useOutsideEvent');

describe('PopupHighlightError', () => {
const defaults = {
onCancel: jest.fn(),
shape: {
height: 10,
width: 0,
Expand All @@ -16,6 +20,25 @@ describe('PopupHighlightError', () => {

const getWrapper = (props = {}): ShallowWrapper => shallow(<PopupHighlightError {...defaults} {...props} />);

const divRef = React.createRef();

beforeEach(() => {
jest.spyOn(React, 'useRef').mockImplementationOnce(() => ({
current: {
popupRef: divRef,
},
}));
});

describe('event handlers', () => {
test('should call onCancel', () => {
getWrapper();

expect(defaults.onCancel).toHaveBeenCalled();
expect(useOutsideEvent).toHaveBeenCalledWith('mousedown', divRef, defaults.onCancel);
});
});

describe('render()', () => {
test('should render correct rect and message', () => {
const wrapper = getWrapper();
Expand Down
16 changes: 13 additions & 3 deletions src/highlight/HighlightAnnotations.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import PopupHighlightError from '../components/Popups/PopupHighlightError';
import useWindowSize from '../common/useWindowSize';
import { AnnotationHighlight } from '../@types';
import { CreateArg } from './actions';
import { CreatorItemHighlight, CreatorStatus, SelectionItem } from '../store';
import { CreatorItemHighlight, CreatorStatus, SelectionArg, SelectionItem } from '../store';
import { getBoundingRect, getShapeRelativeToContainer } from './highlightUtil';
import './HighlightAnnotations.scss';

Expand All @@ -24,6 +24,7 @@ type Props = {
setActiveAnnotationId: (annotationId: string | null) => void;
setIsPromoting: (isPromoting: boolean) => void;
setReferenceShape: (rect: DOMRect) => void;
setSelection: (selection: SelectionArg | null) => void;
setStaged: (staged: CreatorItemHighlight | null) => void;
setStatus: (status: CreatorStatus) => void;
staged?: CreatorItemHighlight | null;
Expand All @@ -40,6 +41,7 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
setActiveAnnotationId,
setIsPromoting,
setReferenceShape,
setSelection,
setStaged,
setStatus,
staged,
Expand Down Expand Up @@ -76,6 +78,10 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
setIsPromoting(true);
};

const handleCancel = (): void => {
setSelection(null);
};

React.useEffect(() => {
if (!isSelecting) {
return;
Expand Down Expand Up @@ -119,14 +125,18 @@ const HighlightAnnotations = (props: Props): JSX.Element => {
{/* Layer 3a: Annotations promoter to promote selection to staged */}
{!isCreating && selection && !selection.hasError && (
<div className="ba-HighlightAnnotations-popup">
<PopupHighlight onClick={handlePromote} shape={getBoundingRect(selection.rects)} />
<PopupHighlight
onCancel={handleCancel}
onClick={handlePromote}
shape={getBoundingRect(selection.rects)}
/>
</div>
)}

{/* Layer 3b: Highlight error popup */}
{selection && selection.hasError && (
<div className="ba-HighlightAnnotations-popup">
<PopupHighlightError shape={getBoundingRect(selection.rects)} />
<PopupHighlightError onCancel={handleCancel} shape={getBoundingRect(selection.rects)} />
</div>
)}
</>
Expand Down
2 changes: 2 additions & 0 deletions src/highlight/HighlightContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
setActiveAnnotationIdAction,
setIsPromotingAction,
setReferenceShapeAction,
setSelectionAction,
setStagedAction,
setStatusAction,
} from '../store';
Expand Down Expand Up @@ -51,6 +52,7 @@ export const mapDispatchToProps = {
setActiveAnnotationId: setActiveAnnotationIdAction,
setIsPromoting: setIsPromotingAction,
setReferenceShape: setReferenceShapeAction,
setSelection: setSelectionAction,
setStaged: setStagedAction,
setStatus: setStatusAction,
};
Expand Down
10 changes: 10 additions & 0 deletions src/highlight/__tests__/HighlightAnnotations-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('HighlightAnnotations', () => {
setActiveAnnotationId: jest.fn(),
setIsPromoting: jest.fn(),
setReferenceShape: jest.fn(),
setSelection: jest.fn(),
setStaged: jest.fn(),
setStatus: jest.fn(),
staged: null,
Expand Down Expand Up @@ -153,6 +154,15 @@ describe('HighlightAnnotations', () => {
});
});

describe('handleCancel()', () => {
test('should clear selection in store', () => {
const wrapper = getWrapper({ selection: selectionMock });
(wrapper.find(PopupHighlight).prop('onCancel') as Function)();

expect(defaults.setSelection).toHaveBeenCalledWith(null);
});
});

describe('effects', () => {
describe('Beginning a selection', () => {
test('should reset staged and status if isSelecting is true', () => {
Expand Down