From 75a425e3eb88cab4f9ff70bc5dedc85059b4e7ee Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Wed, 18 Oct 2023 12:23:21 +0100 Subject: [PATCH 1/2] refactor Dialog to use internally --- src/ConfirmationDialog/ConfirmationDialog.tsx | 1 + src/Dialog/Dialog.tsx | 173 ++++++++---------- src/utils/test-helpers.tsx | 9 + 3 files changed, 86 insertions(+), 97 deletions(-) diff --git a/src/ConfirmationDialog/ConfirmationDialog.tsx b/src/ConfirmationDialog/ConfirmationDialog.tsx index fa951fd6962..e2fa9f4c9c1 100644 --- a/src/ConfirmationDialog/ConfirmationDialog.tsx +++ b/src/ConfirmationDialog/ConfirmationDialog.tsx @@ -156,6 +156,7 @@ async function confirm(themeProps: ThemeProviderProps, options: ConfirmOptions): const root = createRoot(hostElement) const onClose: ConfirmationDialogProps['onClose'] = gesture => { root.unmount() + if (gesture === 'confirm') { resolve(true) } else { diff --git a/src/Dialog/Dialog.tsx b/src/Dialog/Dialog.tsx index aa00fed479f..ff98377696d 100644 --- a/src/Dialog/Dialog.tsx +++ b/src/Dialog/Dialog.tsx @@ -1,19 +1,18 @@ -import React, {useCallback, useEffect, useRef, useState} from 'react' +import React, {useCallback, useRef} from 'react' import styled from 'styled-components' import {Button, ButtonProps} from '../Button' import Box from '../Box' import {get} from '../constants' -import {useOnEscapePress, useProvidedRefOrCreate} from '../hooks' -import {useFocusTrap} from '../hooks/useFocusTrap' import sx, {SxProp} from '../sx' import Octicon from '../Octicon' import {XIcon} from '@primer/octicons-react' import {useFocusZone} from '../hooks/useFocusZone' import {FocusKeys} from '@primer/behaviors' -import Portal from '../Portal' -import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' +import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' import {useId} from '../hooks/useId' import {ScrollableRegion} from '../internal/components/ScrollableRegion' +import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate' +import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' /* Dialog Version 2 */ @@ -96,13 +95,23 @@ export interface DialogProps extends SxProp { footerButtons?: DialogButtonProps[] /** - * This method is invoked when a gesture to close the dialog is used (either - * an Escape key press or clicking the "X" in the top-right corner). The - * gesture argument indicates the gesture that was used to close the dialog - * (either 'close-button' or 'escape'). + * This method is invoked when the dialog has been closed. This could + * be from the Dialog. + * @param gesture - Deprecated: The gesture argument was used to + * indicate if the gesture was from the close-button or escape button. + * It will always be `'close-button'`, to check for _cancelations_ of + * the dialog using the Esc key, use onCancel. */ onClose: (gesture: 'close-button' | 'escape') => void + /** + * This method is invoked when the user instructs the browser they wish to + * dismiss the dialog. Typically this means they have pressed the `Esc` key. + * + * @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/cancel_event + */ + onCancel?: () => void + /** * Default: "dialog". The ARIA role to assign to this dialog. * @see https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal @@ -126,6 +135,11 @@ export interface DialogProps extends SxProp { * auto: variable based on contents */ height?: DialogHeight + + /** + * Whether or not the Dialog is rendered initially open + */ + initiallyOpen?: boolean } /** @@ -145,32 +159,11 @@ export interface DialogHeaderProps extends DialogProps { dialogDescriptionId: string } -const Backdrop = styled('div')` - position: fixed; - top: 0; - left: 0; - bottom: 0; - right: 0; - display: flex; - align-items: center; - justify-content: center; - background-color: ${get('colors.primer.canvas.backdrop')}; - animation: dialog-backdrop-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')}; - - @keyframes dialog-backdrop-appear { - 0% { - opacity: 0; - } - 100% { - opacity: 1; - } - } -` - const heightMap = { small: '480px', large: '640px', auto: 'auto', + 'min-content': 'min-content', } as const const widthMap = { @@ -188,19 +181,40 @@ type StyledDialogProps = { height?: DialogHeight } & SxProp -const StyledDialog = styled.div` - display: flex; +const StyledDialog = styled.dialog` flex-direction: column; background-color: ${get('colors.canvas.overlay')}; + color: ${get('colors.fg.default')}; box-shadow: ${get('shadows.overlay.shadow')}; min-width: 296px; max-width: calc(100vw - 64px); max-height: calc(100vh - 64px); width: ${props => widthMap[props.width ?? ('xlarge' as const)]}; height: ${props => heightMap[props.height ?? ('auto' as const)]}; + border: 0; border-radius: 12px; + padding: 0; opacity: 1; animation: overlay--dialog-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')}; + overflow: initial; + + &[open] { + display: flex; + } + + &::backdrop { + background-color: ${get('colors.primer.canvas.backdrop')}; + animation: dialog-backdrop-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')}; + } + + @keyframes dialog-backdrop-appear { + 0% { + opacity: 0; + } + 100% { + opacity: 1; + } + } @keyframes overlay--dialog-appear { 0% { @@ -253,7 +267,7 @@ const DefaultFooter: React.FC> = ({footerBu ) : null } -const _Dialog = React.forwardRef>((props, forwardedRef) => { +const _Dialog = React.forwardRef>((props, forwardedRef) => { const { title = 'Dialog', subtitle = '', @@ -261,78 +275,50 @@ const _Dialog = React.forwardRef(null) - for (const footerButton of footerButtons) { - if (footerButton.autoFocus) { - footerButton.ref = autoFocusedFooterButtonRef - } - } const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId} - const dialogRef = useRef(null) + const dialogRef = useRef(null) useRefObjectAsForwardedRef(forwardedRef, dialogRef) - const backdropRef = useRef(null) - useFocusTrap({containerRef: dialogRef, restoreFocusOnCleanUp: true, initialFocusRef: autoFocusedFooterButtonRef}) - - useOnEscapePress( - (event: KeyboardEvent) => { - onClose('escape') - event.preventDefault() - }, - [onClose], - ) - React.useEffect(() => { - const bodyOverflowStyle = document.body.style.overflow || '' - // If the body is already set to overflow: hidden, it likely means - // that there is already a modal open. In that case, we should bail - // so we don't re-enable scroll after the second dialog is closed. - if (bodyOverflowStyle === 'hidden') { - return + useIsomorphicLayoutEffect(() => { + if (initiallyOpen && !dialogRef.current?.open && dialogRef.current?.isConnected) { + dialogRef.current.showModal() } + }, [initiallyOpen, dialogRef]) - document.body.style.overflow = 'hidden' - - return () => { - document.body.style.overflow = bodyOverflowStyle - } - }, []) + const onCloseHandler = useCallback(() => onClose('close-button'), [onClose]) const header = (renderHeader ?? DefaultHeader)(defaultedProps) const body = (renderBody ?? DefaultBody)(defaultedProps) const footer = (renderFooter ?? DefaultFooter)(defaultedProps) return ( - <> - - - - {header} - - {body} - - {footer} - - - - + + {header} + + {body} + + {footer} + ) }) _Dialog.displayName = 'Dialog' @@ -384,16 +370,9 @@ const Footer = styled.div` const Buttons: React.FC> = ({buttons}) => { const autoFocusRef = useProvidedRefOrCreate(buttons.find(button => button.autoFocus)?.ref) let autoFocusCount = 0 - const [hasRendered, setHasRendered] = useState(0) - useEffect(() => { - // hack to work around dialogs originating from other focus traps. - if (hasRendered === 1) { - autoFocusRef.current?.focus() - } else { - setHasRendered(hasRendered + 1) - } - }, [autoFocusRef, hasRendered]) - + useIsomorphicLayoutEffect(() => { + autoFocusRef.current?.setAttribute('autofocus', '') + }, [autoFocusRef]) return ( <> {buttons.map((dialogButtonProps, index) => { diff --git a/src/utils/test-helpers.tsx b/src/utils/test-helpers.tsx index bcca7a6d1cc..b73c29a259a 100644 --- a/src/utils/test-helpers.tsx +++ b/src/utils/test-helpers.tsx @@ -18,3 +18,12 @@ global.CSS = { } global.TextEncoder = TextEncoder + +// Dialog showModal isn't implemented in JSDOM https://github.com/jsdom/jsdom/issues/3294 +global.HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) { + // eslint-disable-next-line no-invalid-this + this.open = true + // eslint-disable-next-line no-invalid-this + const element: HTMLElement | null = this.querySelector('[autofocus]') + element?.focus() +}) From 51c8194c8df40a04851c0183a0bb5a630f4e48fa Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 7 Dec 2023 12:16:52 +0000 Subject: [PATCH 2/2] changeset --- .changeset/little-dryers-sort.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/little-dryers-sort.md diff --git a/.changeset/little-dryers-sort.md b/.changeset/little-dryers-sort.md new file mode 100644 index 00000000000..b8192f2fecf --- /dev/null +++ b/.changeset/little-dryers-sort.md @@ -0,0 +1,7 @@ +--- +'@primer/react': patch +--- + +Dialog now uses + +