Commit f7706eb9 authored by Robert Knight's avatar Robert Knight

Remove support for passing a non-Element ref to `useElementShouldClose`

`useElementShouldClose` allowed passing a ref to a Preact component
rather than a ref to a DOM element. Support for this relied on Preact
component's having a `base` property, which is going away in future [1]

All of the actual usage of `useElementShouldClose` in the app passed an
Element ref, so I've simply removed the functionality and associated
tests.

[1] https://github.com/preactjs/preact/pull/2971
parent af7cd288
......@@ -49,7 +49,6 @@ function TagEditor({
// Set up callback to monitor outside click events to close the AutocompleteList
const closeWrapperRef = useRef(/** @type {HTMLElement|null} */ (null));
/** @ts-ignore - TODO: fix useElementShouldClose Ref types */
useElementShouldClose(closeWrapperRef, suggestionsListOpen, () => {
setSuggestionsListOpen(false);
});
......
......@@ -2,21 +2,27 @@ import { mount } from 'enzyme';
import { createElement } from 'preact';
import { useRef } from 'preact/hooks';
import { act } from 'preact/test-utils';
import propTypes from 'prop-types';
import useElementShouldClose from '../use-element-should-close';
describe('hooks.useElementShouldClose', () => {
describe('useElementShouldClose', () => {
let handleClose;
let e;
const createEvent = (name, props) => {
const event = new Event(name);
Object.assign(event, props);
return event;
};
const events = [
new Event('mousedown'),
new Event('click'),
((e = new Event('keydown')), (e.key = 'Escape'), e),
createEvent('keydown', { key: 'Escape' }),
new Event('focus'),
];
// Create a fake component to mount in tests that uses the hook
// eslint-disable-next-line react/prop-types
function FakeComponent({ isOpen = true }) {
const myRef = useRef();
useElementShouldClose(myRef, isOpen, handleClose);
......@@ -27,56 +33,17 @@ describe('hooks.useElementShouldClose', () => {
);
}
FakeComponent.propTypes = {
isOpen: propTypes.bool,
};
// Tests useElementShouldClose on a custom component directly
function FakeCompoundComponent({ isOpen = true }) {
function FakeCustomComponent() {
return (
<div>
<button>Hi</button>
</div>
);
}
const myRef = useRef();
useElementShouldClose(myRef, isOpen, handleClose);
return <FakeCustomComponent ref={myRef} />;
}
FakeCompoundComponent.propTypes = {
isOpen: propTypes.bool,
};
function createComponent(props) {
return mount(<FakeComponent isOpen={true} {...props} />);
}
function createCompoundComponent(props) {
return mount(<FakeCompoundComponent isOpen={true} {...props} />);
}
beforeEach(() => {
handleClose = sinon.stub();
});
// Run each set of tests twice, once for a regular node and a second
// time for a custom preact component
[
{
createWrapper: createComponent,
description: 'useElementShouldClose attached to a html node',
},
{
createWrapper: createCompoundComponent,
description: 'useElementShouldClose attached to a preact component',
},
].forEach(test => {
context(test.description, () => {
events.forEach(event => {
it(`should invoke close callback once for events outside of element (${event.type})`, () => {
const wrapper = test.createWrapper();
const wrapper = createComponent();
act(() => {
document.body.dispatchEvent(event);
......@@ -100,7 +67,7 @@ describe('hooks.useElementShouldClose', () => {
events.forEach(event => {
it(`should not invoke close callback on events outside of element if element closed (${event.type})`, () => {
const wrapper = test.createWrapper({ isOpen: false });
const wrapper = createComponent({ isOpen: false });
act(() => {
document.body.dispatchEvent(event);
......@@ -113,7 +80,7 @@ describe('hooks.useElementShouldClose', () => {
events.forEach(event => {
it(`should not invoke close callback on events inside of element (${event.type})`, () => {
const wrapper = test.createWrapper();
const wrapper = createComponent();
const button = wrapper.find('button');
act(() => {
......@@ -124,6 +91,4 @@ describe('hooks.useElementShouldClose', () => {
assert.equal(handleClose.callCount, 0);
});
});
});
});
});
......@@ -8,15 +8,6 @@ import { listen } from '../../util/dom';
* @typedef {import("preact/hooks").Ref<T>} Ref
*/
/**
* @typedef PreactElement
* @prop {Node} base
*/
/**
* @typedef {Ref<HTMLElement> | Ref<PreactElement>} PreactRef
*/
/**
* This hook adds appropriate `eventListener`s to the document when a target
* element (`closeableEl`) is open. Events such as `click` and `focus` on
......@@ -25,12 +16,8 @@ import { listen } from '../../util/dom';
* to indicate that `closeableEl` should be closed. This hook also performs
* cleanup to remove `eventListener`s when appropriate.
*
* Limitation: This will not work when attached to a custom component that has
* more than one element nested under a root <Fragment>
*
* @param {PreactRef} closeableEl - ref object:
* Reference to a DOM element or preat component
* that should be closed when DOM elements external
* @param {Ref<HTMLElement>} closeableEl -
* Reference to a DOM element that should be closed when DOM elements external
* to it are interacted with or `Esc` is pressed
* @param {boolean} isOpen - Whether the element is currently open. This hook does
* not attach event listeners/do anything if it's not.
......@@ -42,25 +29,6 @@ export default function useElementShouldClose(
isOpen,
handleClose
) {
/**
* Helper to return the underlying node object whether
* `closeableEl` is attached to an HTMLNode or Preact component.
*
* @param {PreactRef} closeableEl
* @returns {Node}
*/
const getCurrentNode = closeableEl => {
// if base is present, assume its a preact component
const node = /** @type {PreactElement} */ (closeableEl.current).base
? /** @type {PreactElement} */ (closeableEl.current).base
: closeableEl.current;
if (typeof node !== 'object') {
throw new Error('useElementShouldClose can not find a node reference');
}
return /** @type {Node} */ (node);
};
useEffect(() => {
if (!isOpen) {
return () => {};
......@@ -80,8 +48,7 @@ export default function useElementShouldClose(
document.body,
'focus',
event => {
const current = getCurrentNode(closeableEl);
if (!current.contains(/** @type {Node} */ (event.target))) {
if (!closeableEl.current.contains(/** @type {Node} */ (event.target))) {
handleClose();
}
},
......@@ -94,8 +61,7 @@ export default function useElementShouldClose(
document.body,
['mousedown', 'click'],
event => {
const current = getCurrentNode(closeableEl);
if (!current.contains(/** @type {Node} */ (event.target))) {
if (!closeableEl.current.contains(/** @type {Node} */ (event.target))) {
handleClose();
}
},
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment