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

Remove usages of deprecated useElementShouldClose #6464

Closed
wants to merge 1 commit into from
Closed
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
16 changes: 11 additions & 5 deletions src/sidebar/components/Annotation/AnnotationShareControl.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { useElementShouldClose } from '@hypothesis/frontend-shared';
import {
useClickAway,
useFocusAway,
useKeyPress,
} from '@hypothesis/frontend-shared';
import {
Card,
IconButton,
Expand Down Expand Up @@ -62,8 +66,10 @@ function AnnotationShareControl({
const toggleSharePanel = () => setOpen(!isOpen);
const closePanel = () => setOpen(false);

// Interactions outside of the component when it is open should close it
useElementShouldClose(shareRef, isOpen, closePanel);
// Interactions outside the component when it is open should close it
useClickAway(shareRef, closePanel, { enabled: isOpen });
useFocusAway(shareRef, closePanel, { enabled: isOpen });
useKeyPress(['Escape'], closePanel, { enabled: isOpen });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hypothesis/frontend-shared#900 documented the rationale for decomposing the useElementShouldClose hook into separate hooks, but I think it would be useful to have a combined hook that captured the set of actions that close popover-type controls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, maybe you are right. Perhaps what we should do is keep useElementShouldClose, un-deprecate it, and just make it a wrapper of the other three hooks.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would add a new usePopoverShouldClose hook that wraps the existing hooks and is more specific about the kind of widget it works with. The problem with useElementShouldClose was that we used it for popover-type controls but also different controls that needed different close behaviors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. I'll close this one for now


useEffect(() => {
if (wasOpen.current !== isOpen) {
Expand Down Expand Up @@ -129,8 +135,8 @@ function AnnotationShareControl({
return (
// Make the container div focusable by setting a non-null `tabIndex`.
// This prevents clicks on non-focusable contents from "leaking out" and
// focusing a focusable ancester. If something outside of the panel gains
// focus, `useElementShouldClose`'s focus listener will close the panel.
// focusing a focusable ancester. If something outside the panel gains
// focus, `useFocusAway`'s focus listener will close the panel.
// "Catch focus" here to prevent this.
// See https://github.com/hypothesis/client/issues/5196
<div className="relative" ref={shareRef} tabIndex={-1}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ describe('AnnotationShareControl', () => {
$imports.$mock(mockImportedComponents());
$imports.$mock({
'@hypothesis/frontend-shared': {
useElementShouldClose: sinon.stub(),
useClickAway: sinon.stub(),
useFocusAway: sinon.stub(),
useKeyPress: sinon.stub(),
},
'../../helpers/annotation-sharing': {
isShareableURI: fakeIsShareableURI,
Expand Down
10 changes: 8 additions & 2 deletions src/sidebar/components/Menu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { useElementShouldClose } from '@hypothesis/frontend-shared';
import {
useKeyPress,
useClickAway,
useFocusAway,
} from '@hypothesis/frontend-shared';
import { MenuExpandIcon } from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import type { ComponentChildren } from 'preact';
Expand Down Expand Up @@ -145,7 +149,9 @@ export default function Menu({

// Menu element should close via `closeMenu` whenever it's open and there
// are user interactions outside of it (e.g. clicks) in the document
useElementShouldClose(menuRef, isOpen, closeMenu);
useClickAway(menuRef, closeMenu, { enabled: isOpen });
useFocusAway(menuRef, closeMenu, { enabled: isOpen });
useKeyPress(['Escape'], closeMenu, { enabled: isOpen });

const stopPropagation = (e: Event) => e.stopPropagation();

Expand Down
17 changes: 14 additions & 3 deletions src/sidebar/components/TagEditor.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { useElementShouldClose } from '@hypothesis/frontend-shared';
import {
useClickAway,
useFocusAway,
useKeyPress,
} from '@hypothesis/frontend-shared';
import { Input } from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import { useRef, useState } from 'preact/hooks';
Expand Down Expand Up @@ -45,8 +49,14 @@ function TagEditor({

// Set up callback to monitor outside click events to close the AutocompleteList
const closeWrapperRef = useRef<HTMLDivElement>(null);
useElementShouldClose(closeWrapperRef, suggestionsListOpen, () => {
setSuggestionsListOpen(false);
useClickAway(closeWrapperRef, () => setSuggestionsListOpen(false), {
enabled: suggestionsListOpen,
});
useFocusAway(inputEl, () => setSuggestionsListOpen(false), {
enabled: suggestionsListOpen,
});
useKeyPress(['Escape'], () => setSuggestionsListOpen(false), {
enabled: suggestionsListOpen,
});

/**
Expand Down Expand Up @@ -282,6 +292,7 @@ function TagEditor({
// Larger font on touch devices
'text-base touch:text-touch-base',
)}
data-testid="input"
/>
<AutocompleteList
id={`${tagEditorId}-AutocompleteList`}
Expand Down
15 changes: 14 additions & 1 deletion src/sidebar/components/test/Menu-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ describe('Menu', () => {
new Event('mousedown'),
new Event('click'),
((e = new Event('keydown')), (e.key = 'Escape'), e),
new Event('focus'),
].forEach(event => {
it(`closes when the user clicks or presses the mouse outside (${event.type})`, () => {
const wrapper = createMenu({ defaultOpen: true });
Expand All @@ -126,6 +125,20 @@ describe('Menu', () => {
});
});

it('closes when menu loses focus', () => {
const wrapper = createMenu({ defaultOpen: true });

act(() => {
wrapper
.find(menuSelector)
.getDOMNode()
.dispatchEvent(new Event('focusout'));
});
wrapper.update();

assert.isFalse(isOpen(wrapper));
});

it('does not close when user presses non-Escape key outside', () => {
const wrapper = createMenu({ defaultOpen: true });

Expand Down
5 changes: 4 additions & 1 deletion src/sidebar/components/test/TagEditor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ describe('TagEditor', () => {
wrapper.find('input').instance().value = 'non-empty';
typeInput(wrapper);
assert.equal(wrapper.find('AutocompleteList').prop('open'), true);
document.body.dispatchEvent(new Event('focus'));
wrapper
.find('input[data-testid="input"]')
.getDOMNode()
.dispatchEvent(new Event('focusout'));
wrapper.update();
assert.equal(wrapper.find('AutocompleteList').prop('open'), false);
});
Expand Down