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

Make close behavior for Dialogs more granular, configurable #912

Merged
merged 2 commits into from
Mar 10, 2023
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
33 changes: 28 additions & 5 deletions src/components/feedback/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import type { RefObject } from 'preact';
import { Fragment } from 'preact';
import { useEffect, useLayoutEffect } from 'preact/hooks';

import { useElementShouldClose } from '../../hooks/use-element-should-close';
import { useClickAway } from '../../hooks/use-click-away';
import { useFocusAway } from '../../hooks/use-focus-away';
import { useKeyPress } from '../../hooks/use-key-press';
import { useSyncedRef } from '../../hooks/use-synced-ref';
import { useUniqueId } from '../../hooks/use-unique-id';
import type { PresentationalProps } from '../../types';
Expand All @@ -13,6 +15,9 @@ import Panel from '../layout/Panel';
import type { PanelProps } from '../layout/Panel';

type ComponentProps = {
closeOnClickAway?: boolean;
closeOnEscape?: boolean;
closeOnFocusAway?: boolean;
/**
* Dialog _should_ be provided with a close handler. We have a few edge use
* cases, however, in which we need to render a "non-closeable" modal dialog.
Expand Down Expand Up @@ -43,16 +48,19 @@ export type DialogProps = PresentationalProps &
const noop = () => {};

/**
* Show a modal
* Show a dialog
*/
const DialogNext = function Dialog({
closeOnClickAway = false,
closeOnEscape, // Default depends on whether modal or not
closeOnFocusAway = false,
children,
width = 'md',
initialFocus = 'auto',
modal = false,

classes,
elementRef,
initialFocus = 'auto',
modal = false,

// Forwarded to Panel
buttons,
Expand All @@ -66,7 +74,22 @@ const DialogNext = function Dialog({
}: DialogProps) {
const modalRef = useSyncedRef(elementRef);
const closeHandler = onClose ?? noop;
useElementShouldClose(modalRef, true, closeHandler);

// Closing when ESC pressed is default behavior for modal dialogs
const closeOnEscapeEnabled =
(modal && closeOnEscape !== false) || closeOnEscape === true;

useClickAway(modalRef, () => closeHandler(), {
enabled: closeOnClickAway,
});

useKeyPress(['Escape'], () => closeHandler(), {
enabled: closeOnEscapeEnabled,
});

useFocusAway(modalRef, () => closeHandler(), {
enabled: closeOnFocusAway,
});

const dialogDescriptionId = useUniqueId('dialog-description');

Expand Down
118 changes: 109 additions & 9 deletions src/components/feedback/test/Dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { createRef } from 'preact';

import { testPresentationalComponent } from '../../test/common-tests';
import Dialog from '../Dialog';
import { $imports } from '../Dialog';

const createComponent = (Component, props = {}) => {
return mount(
Expand All @@ -13,11 +14,36 @@ const createComponent = (Component, props = {}) => {
};

describe('Dialog', () => {
let fakeUseClickAway;
let fakeUseFocusAway;
let fakeUseKeyPress;

testPresentationalComponent(Dialog, {
createContent: createComponent,
elementSelector: 'div[data-component="Dialog"]',
});

beforeEach(() => {
fakeUseClickAway = sinon.stub();
fakeUseFocusAway = sinon.stub();
fakeUseKeyPress = sinon.stub();
$imports.$mock({
'../../hooks/use-click-away': {
useClickAway: fakeUseClickAway,
},
'../../hooks/use-focus-away': {
useFocusAway: fakeUseFocusAway,
},
'../../hooks/use-key-press': {
useKeyPress: fakeUseKeyPress,
},
});
});

afterEach(() => {
$imports.$restore();
});

describe('initial focus', () => {
let container;

Expand Down Expand Up @@ -87,24 +113,98 @@ describe('Dialog', () => {
});

describe('closing the dialog', () => {
it('closes when `ESC` pressed', () => {
const onClose = sinon.stub();
const container = document.createElement('div');
let container;
let onClose;

beforeEach(() => {
onClose = sinon.stub();
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
container.remove();
});

context('modal dialog', () => {
it('enables close on `ESC`, but not on external click or focus', () => {
mount(
<Dialog title="Test dialog" modal onClose={onClose}>
Modal dialog
</Dialog>,
{
attachTo: container,
}
);

assert.deepEqual(fakeUseKeyPress.lastCall.args[0], ['Escape']);
assert.deepEqual(fakeUseKeyPress.lastCall.args[2], { enabled: true });
assert.deepEqual(fakeUseFocusAway.lastCall.args[2], { enabled: false });
assert.deepEqual(fakeUseClickAway.lastCall.args[2], { enabled: false });
});

it('does not enable close on `ESC` if overridden by `closeOnEscape`', () => {
mount(
<Dialog
closeOnEscape={false}
title="Test dialog"
modal
onClose={onClose}
>
Modal dialog
</Dialog>,
{
attachTo: container,
}
);

assert.deepEqual(fakeUseKeyPress.lastCall.args[0], ['Escape']);
assert.deepEqual(fakeUseKeyPress.lastCall.args[2], { enabled: false });
});
});

context('non-modal dialog', () => {
it('does not enable close on ESC, external clicks or external focus events', () => {
mount(
<Dialog title="Test dialog" onClose={onClose}>
This is my dialog
</Dialog>,
{
attachTo: container,
}
);

assert.deepEqual(fakeUseKeyPress.lastCall.args[0], ['Escape']);
assert.deepEqual(fakeUseKeyPress.lastCall.args[2], { enabled: false });
assert.deepEqual(fakeUseFocusAway.lastCall.args[2], { enabled: false });
assert.deepEqual(fakeUseClickAway.lastCall.args[2], { enabled: false });
});
});

it('enables close on external click if `closeOnClickAway` set', () => {
mount(
<Dialog title="Test dialog" onClose={onClose}>
<Dialog title="Test dialog" closeOnClickAway onClose={onClose}>
This is my dialog
</Dialog>,
{
attachTo: container,
}
);

const event = new Event('keydown');
event.key = 'Escape';
document.body.dispatchEvent(event);
assert.called(onClose);
container.remove();
assert.deepEqual(fakeUseClickAway.lastCall.args[2], { enabled: true });
});

it('enables close on external focus if `closeOnFocusAway` set', () => {
mount(
<Dialog title="Test dialog" closeOnFocusAway onClose={onClose}>
This is my dialog
</Dialog>,
{
attachTo: container,
}
);

assert.deepEqual(fakeUseFocusAway.lastCall.args[2], { enabled: true });
});
});

Expand Down
90 changes: 82 additions & 8 deletions src/pattern-library/components/patterns/feedback/DialogPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,23 +97,23 @@ export default function DialogPage() {
Differentiation between modal and non-modal Dialogs via a{' '}
<code>modal</code> prop.
</li>
</ul>
</Library.Example>

<Library.Example title="TODO">
<ul>
<li>
Close on ESC keypress: Defaults to enabled for modal dialogs,
off for non-modal dialogs. Can be controlled with a prop.
</li>
<li>
Close on click-away: Defaults to enabled for modal dialogs, off
for non-modal dialogs. Can be controlled with a prop.
Close on click-away: Defaults to off for all dialogs. Can be
controlled with a prop.
</li>
<li>
Close on focus-away: Defaults to off for all dialogs. Can be
conrolled with a prop.
controlled with a prop.
</li>
</ul>
</Library.Example>

<Library.Example title="TODO">
<ul>
<li>
Focus trap: Defaults to enabled for modal dialogs, off for
non-modal dialogs. Can be controlled with a prop.
Expand Down Expand Up @@ -155,6 +155,11 @@ export default function DialogPage() {
</Dialog_>
</Library.Demo>

<p>
Modal dialogs position themselves atop a full-screen overlay, and
will close by default when <kbd>Escape</kbd> is pressed.
</p>

<Library.Demo title="Basic modal Dialog" withSource>
<Dialog_
buttons={<DialogButtons />}
Expand Down Expand Up @@ -199,6 +204,75 @@ export default function DialogPage() {
</li>
</ul>
</Library.Example>
<Library.Example title="closeOnClickAway">
<p>
This boolean prop (default <code>false</code>) controls whether
the Dialog should close when there are click events outside of it.
</p>
<Library.Demo
title="Dialog that closes on external clicks"
withSource
>
<Dialog_
closeOnClickAway
onClose={() => {}}
title="Dialog that closes when there are external clicks"
>
<p>This dialog will close if you click outside of it</p>
</Dialog_>
</Library.Demo>
</Library.Example>
<Library.Example title="closeOnFocusAway">
<p>
This boolean prop (default <code>false</code>) controls whether
the Dialog should close when there are focus events outside of it.
</p>
<Library.Demo
title="Dialog that closes on external focus events"
withSource
>
<Dialog_
closeOnFocusAway
onClose={() => {}}
title="Close on Away Focus"
>
<p>This dialog will close if you focus outside of it</p>
</Dialog_>
</Library.Demo>
</Library.Example>
<Library.Example title="closeOnEscape">
<p>
Close-on-<kbd>Escape</kbd> keypress behavior is enabled by default
when <code>modal</code> is set. The default behavior may be
overridden by setting this prop:
</p>
<ul>
<li>
Set to <code>true</code> to explicitly enable closing on{' '}
<code>Escape</code>, e.g. on non-modal Dialogs
</li>
<li>
Set to <code>false</code> to explicitly disable closing on{' '}
<code>Escape</code>, e.g. on modal Dialogs
</li>
</ul>

<Library.Demo
title="Disabling close-on-Escape for a modal dialog"
withSource
>
<Dialog_
closeOnEscape={false}
modal
onClose={() => {}}
title="ESC won't close me!"
>
<p>
This dialog will not close if you press <kbd>Escape</kbd>.
</p>
</Dialog_>
</Library.Demo>
</Library.Example>
</Library.Pattern>
</Library.Section>
</Library.Page>
Expand Down