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

Enhance Dialog so that it allows providing a TransitionComponent #957

Merged
merged 5 commits into from
Apr 17, 2023
Merged
Changes from 1 commit
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
137 changes: 94 additions & 43 deletions src/components/feedback/Dialog.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import classnames from 'classnames';
import type { RefObject } from 'preact';
import { useEffect, useLayoutEffect, useRef } from 'preact/hooks';
import type { JSX } from 'preact';
import { Fragment } from 'preact';
import {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
} from 'preact/hooks';

import { useClickAway } from '../../hooks/use-click-away';
import { useFocusAway } from '../../hooks/use-focus-away';
Expand All @@ -12,6 +21,11 @@ import { downcastRef } from '../../util/typing';
import Panel from '../layout/Panel';
import type { PanelProps } from '../layout/Panel';

type TransitionComponent = JSX.ElementType<{
visible: boolean;
onTransitionEnd?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in voice chat, perhaps

Suggested change
onTransitionEnd?: () => void;
onTransitionEnd?: (direction:'in'|'out') => void;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

}>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have defined the type here inline, but we can extract it if we think it's soon going to be used somewhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Extract and export, please!


type ComponentProps = {
closeOnClickAway?: boolean;
closeOnEscape?: boolean;
Expand All @@ -33,6 +47,15 @@ type ComponentProps = {
* Restore focus to previously-focused element when unmounted/closed
*/
restoreFocus?: boolean;

/**
* Providing this has the next implications:
* - The component will be used to wrap the Dialog contents.
* - If initialFocus === 'auto', the Dialog will be focused once the open
* transition has finished.
* - onClose will be invoked after the close transition has finished.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to tighten down this comment before we land this as most of this is implementation detail. The consumer probably doesn't need to care about most of this :)

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, good point. It sounded good in my head 😅

transitionComponent?: TransitionComponent;
};

// This component forwards a number of props on to `Panel` but always sets the
Expand All @@ -53,6 +76,7 @@ const DialogNext = function Dialog({
children,
initialFocus = 'auto',
restoreFocus = false,
transitionComponent: TransitionComponent,

classes,
elementRef,
Expand All @@ -68,31 +92,22 @@ const DialogNext = function Dialog({
...htmlAttributes
}: DialogProps) {
const modalRef = useSyncedRef(elementRef);
const closeHandler = onClose ?? noop;
const restoreFocusEl = useRef<HTMLElement | null>(
document.activeElement as HTMLElement | null
);

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

useKeyPress(['Escape'], closeHandler, {
enabled: closeOnEscape,
});

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

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

useEffect(() => {
const [visible, setVisible] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for keeping Dialog stateless from an external point of view!

Copy link
Contributor

Choose a reason for hiding this comment

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

Might consider naming this something that clearly associates it with transitions. A Dialog is always "open" (it's stateless), so this naming of "visible" could be confusing to future devs, especially when there is no transition provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! I'm going to use transitionComponentVisible instead, to make it clear that this state is used with the transitionComponent.

// If a TransitionComponent was provided, closing the Dialog should just set
// it to not visible. The TransitionComponent will take care of actually
// closing the dialog once transition has finished
const closeHandler = TransitionComponent
? () => setVisible(false)
: onClose ?? noop;
const setInitialFocus = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the name of this function is totally accurate, you could consider renaming it something more contextual, like initializeDialog. That would represent it as a sort of bag of operations that get executed once when the Dialog is mounted and "ready" (what "ready" means differs based on whether a TransitionComponent was provided) and might make the code a little easier to scan. OTOH, this really does just deal in initial focus, so perhaps the abstraction is not helpful...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea. It will quickly help realize that anything that needs to happen "at the beginning", should be there, or invoked from there.

Otherwise, if this component evolves, it's easy to end up putting more stuff in the existing useEffect, making it work properly only when there's no TransitionComponent.

I'll rename it.

if (initialFocus === 'manual') {
return;
}
if (initialFocus === 'auto') {
// An explicit `initialFocus` has not be set, so use automatic focus
// An explicit `initialFocus` has not been set, so use automatic focus
Copy link
Contributor

Choose a reason for hiding this comment

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

Hee hee, thanks.

// handling. Modern accessibility guidance is to focus the dialog itself
// rather than trying to be smart about focusing a particular control
// within the dialog.
Expand All @@ -109,8 +124,31 @@ const DialogNext = function Dialog({
} else {
// Fall back to focusing the modal itself
modalRef.current?.focus();
return;
}
}, [initialFocus, modalRef]);

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

useKeyPress(['Escape'], closeHandler, {
enabled: closeOnEscape,
});

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

const dialogDescriptionId = useUniqueId('dialog-description');
const Wrapper = useMemo(
() => TransitionComponent ?? Fragment,
[TransitionComponent]
);

useEffect(() => {
// Trigger initial open animation
setVisible(true);
setInitialFocus();
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed: don't invoke this if a transition has been set.


// We only want to run this effect once when the dialog is mounted.
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -152,31 +190,44 @@ const DialogNext = function Dialog({
);

return (
<div
data-component="Dialog"
tabIndex={-1}
// NB: Role can be overridden with an HTML attribute; this is purposeful
role="dialog"
{...htmlAttributes}
className={classnames(
// Column-flex layout to constrain content to max-height
'flex flex-col',
classes
)}
ref={downcastRef(modalRef)}
<Wrapper
visible={visible}
onTransitionEnd={() => {
Copy link
Contributor

@lyzadanger lyzadanger Apr 13, 2023

Choose a reason for hiding this comment

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

Ah, interesting, so we can rely on a transitionEnd event for elements with no transition defined? I'll have to look that up!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, maybe not:

If there is no transition delay or duration, if both are 0s or neither is declared, there is no transition, and none of the transition events are fired.

https://developer.mozilla.org/en-US/docs/Web/API/Element/transitionend_event

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be easier to reason about/scan if it weren't an inline function.

if (!visible) {
onClose?.();
} else {
// Once transition on a visible Dialog has finished, we re-check the
// initial focus
setInitialFocus();
}
}}
>
<Panel
buttons={buttons}
fullWidthHeader={true}
icon={icon}
onClose={onClose}
paddingSize={paddingSize}
title={title}
scrollable={scrollable}
<div
data-component="Dialog"
tabIndex={-1}
// NB: Role can be overridden with an HTML attribute; this is purposeful
role="dialog"
{...htmlAttributes}
className={classnames(
// Column-flex layout to constrain content to max-height
'flex flex-col',
classes
)}
ref={downcastRef(modalRef)}
>
{children}
</Panel>
</div>
<Panel
buttons={buttons}
fullWidthHeader={true}
icon={icon}
onClose={closeHandler}
paddingSize={paddingSize}
title={title}
scrollable={scrollable}
>
{children}
</Panel>
</div>
</Wrapper>
);
};

Expand Down