From 2e8cdf5b8839f90316336f7bc327f710397f8d3f Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 25 Jul 2022 16:57:12 +0000 Subject: [PATCH 1/4] allow configuration of the initial focus in a confirmation dialog --- docs/content/drafts/Dialog.mdx | 1 + package-lock.json | 4 ++-- src/Dialog/ConfirmationDialog.tsx | 11 ++++++++--- src/__tests__/ConfirmationDialog.test.tsx | 23 ++++++++++++++++++++++- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/docs/content/drafts/Dialog.mdx b/docs/content/drafts/Dialog.mdx index c27726b4ec8..81e59acd4ea 100644 --- a/docs/content/drafts/Dialog.mdx +++ b/docs/content/drafts/Dialog.mdx @@ -174,6 +174,7 @@ render() | cancelButtonContent | `React.ReactNode` | `"Cancel"` | The content to use for the cancel button. | | confirmButtonContent | `React.ReactNode` | `"OK"` | The content to use for the confirm button. | | confirmButtonType | `"normal" │ "primary" │ "danger"` | `Button` | The type of button to use for the confirm button. | +| autoFocusButton | `"confirm" │ "cancel" | `confirm` | The button initially focused when the ConfirmationDialog is opened | ### ConfirmOptions diff --git a/package-lock.json b/package-lock.json index 4b2f0ec4a2a..1b41c0a2d0c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@primer/react", - "version": "35.4.0", + "version": "35.5.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@primer/react", - "version": "35.4.0", + "version": "35.5.0", "license": "MIT", "dependencies": { "@primer/behaviors": "^1.1.1", diff --git a/src/Dialog/ConfirmationDialog.tsx b/src/Dialog/ConfirmationDialog.tsx index 03bd4b9214b..ce630429e9a 100644 --- a/src/Dialog/ConfirmationDialog.tsx +++ b/src/Dialog/ConfirmationDialog.tsx @@ -38,6 +38,9 @@ export interface ConfirmationDialogProps { * The type of button to use for the confirm button. Default: Button. */ confirmButtonType?: 'normal' | 'primary' | 'danger' + + /** The button initially focused when the ConfirmationDialog is opened. Default: "confirm" */ + autoFocusButton?: 'confirm' | 'cancel' } const StyledConfirmationHeader = styled.header` @@ -107,7 +110,8 @@ export const ConfirmationDialog: React.FC = props => { cancelButtonContent = 'Cancel', confirmButtonContent = 'OK', confirmButtonType = 'normal', - children + children, + autoFocusButton = 'confirm' } = props const onCancelButtonClick = useCallback(() => { @@ -118,13 +122,14 @@ export const ConfirmationDialog: React.FC = props => { }, [onClose]) const cancelButton: DialogButtonProps = { content: cancelButtonContent, - onClick: onCancelButtonClick + onClick: onCancelButtonClick, + autoFocus: autoFocusButton === 'cancel' } const confirmButton: DialogButtonProps = { content: confirmButtonContent, buttonType: confirmButtonType, onClick: onConfirmButtonClick, - autoFocus: true + autoFocus: autoFocusButton === 'confirm' } const footerButtons = [cancelButton, confirmButton] return ( diff --git a/src/__tests__/ConfirmationDialog.test.tsx b/src/__tests__/ConfirmationDialog.test.tsx index 7de379edf25..471ead9dfe8 100644 --- a/src/__tests__/ConfirmationDialog.test.tsx +++ b/src/__tests__/ConfirmationDialog.test.tsx @@ -15,7 +15,7 @@ import {behavesAsComponent, checkExports} from '../utils/testing' expect.extend(toHaveNoViolations) -const Basic = () => { +const Basic = ({autoFocusButton = 'confirm'}: {autoFocusButton?: 'confirm' | 'cancel'}) => { const [isOpen, setIsOpen] = useState(false) const buttonRef = useRef(null) const onDialogClose = useCallback(() => setIsOpen(false), []) @@ -32,6 +32,7 @@ const Basic = () => { onClose={onDialogClose} cancelButtonContent="Secondary" confirmButtonContent="Primary" + autoFocusButton={autoFocusButton} > Lorem ipsum dolor sit Pippin good dog. @@ -105,6 +106,26 @@ describe('ConfirmationDialog', () => { cleanup() }) + it('focuses the primary action when opened with autoFocusButton set to confirm', async () => { + const {getByText} = HTMLRender() + act(() => { + fireEvent.click(getByText('Show dialog')) + }) + expect(getByText('Primary')).toEqual(document.activeElement) + expect(getByText('Secondary')).not.toEqual(document.activeElement) + cleanup() + }) + + it('focuses the secondary action when opened with autoFocusButton set to cancel', async () => { + const {getByText} = HTMLRender() + act(() => { + fireEvent.click(getByText('Show dialog')) + }) + expect(getByText('Primary')).not.toEqual(document.activeElement) + expect(getByText('Secondary')).toEqual(document.activeElement) + cleanup() + }) + it('supports nested `focusTrap`s', async () => { const {getByText} = HTMLRender() act(() => { From b707dc67fcc07072e4c4c8b3f2eafb6886d13de9 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 25 Jul 2022 16:58:52 +0000 Subject: [PATCH 2/4] changeset --- .changeset/lucky-pianos-yell.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lucky-pianos-yell.md diff --git a/.changeset/lucky-pianos-yell.md b/.changeset/lucky-pianos-yell.md new file mode 100644 index 00000000000..a119a5a1a9c --- /dev/null +++ b/.changeset/lucky-pianos-yell.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Allow configuration of ConfirmationDialog initial focus From 937e8287f08efa1a1856d719394aacdb550b72f4 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 26 Jul 2022 14:21:31 +0000 Subject: [PATCH 3/4] explicitly set the autoFocus when danger to cancel --- .changeset/lucky-pianos-yell.md | 4 ++-- docs/content/drafts/Dialog.mdx | 1 - src/Dialog/ConfirmationDialog.tsx | 11 ++++------- src/__tests__/ConfirmationDialog.test.tsx | 14 +++++++------- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/.changeset/lucky-pianos-yell.md b/.changeset/lucky-pianos-yell.md index a119a5a1a9c..c4d16e77efb 100644 --- a/.changeset/lucky-pianos-yell.md +++ b/.changeset/lucky-pianos-yell.md @@ -1,5 +1,5 @@ --- -'@primer/react': minor +'@primer/react': patch --- -Allow configuration of ConfirmationDialog initial focus +Set ConfirmationDialog initial focus based on the confirmationButtonVariant. When `danger` autoFocus the cancel button, otherwise autoFocus the confirmation button diff --git a/docs/content/drafts/Dialog.mdx b/docs/content/drafts/Dialog.mdx index 81e59acd4ea..c27726b4ec8 100644 --- a/docs/content/drafts/Dialog.mdx +++ b/docs/content/drafts/Dialog.mdx @@ -174,7 +174,6 @@ render() | cancelButtonContent | `React.ReactNode` | `"Cancel"` | The content to use for the cancel button. | | confirmButtonContent | `React.ReactNode` | `"OK"` | The content to use for the confirm button. | | confirmButtonType | `"normal" │ "primary" │ "danger"` | `Button` | The type of button to use for the confirm button. | -| autoFocusButton | `"confirm" │ "cancel" | `confirm` | The button initially focused when the ConfirmationDialog is opened | ### ConfirmOptions diff --git a/src/Dialog/ConfirmationDialog.tsx b/src/Dialog/ConfirmationDialog.tsx index ce630429e9a..59a3efe9cf3 100644 --- a/src/Dialog/ConfirmationDialog.tsx +++ b/src/Dialog/ConfirmationDialog.tsx @@ -38,9 +38,6 @@ export interface ConfirmationDialogProps { * The type of button to use for the confirm button. Default: Button. */ confirmButtonType?: 'normal' | 'primary' | 'danger' - - /** The button initially focused when the ConfirmationDialog is opened. Default: "confirm" */ - autoFocusButton?: 'confirm' | 'cancel' } const StyledConfirmationHeader = styled.header` @@ -110,8 +107,7 @@ export const ConfirmationDialog: React.FC = props => { cancelButtonContent = 'Cancel', confirmButtonContent = 'OK', confirmButtonType = 'normal', - children, - autoFocusButton = 'confirm' + children } = props const onCancelButtonClick = useCallback(() => { @@ -120,16 +116,17 @@ export const ConfirmationDialog: React.FC = props => { const onConfirmButtonClick = useCallback(() => { onClose('confirm') }, [onClose]) + const cancelButton: DialogButtonProps = { content: cancelButtonContent, onClick: onCancelButtonClick, - autoFocus: autoFocusButton === 'cancel' + autoFocus: confirmButtonType === 'danger' } const confirmButton: DialogButtonProps = { content: confirmButtonContent, buttonType: confirmButtonType, onClick: onConfirmButtonClick, - autoFocus: autoFocusButton === 'confirm' + autoFocus: confirmButtonType !== 'danger' } const footerButtons = [cancelButton, confirmButton] return ( diff --git a/src/__tests__/ConfirmationDialog.test.tsx b/src/__tests__/ConfirmationDialog.test.tsx index 471ead9dfe8..7b62f0b8290 100644 --- a/src/__tests__/ConfirmationDialog.test.tsx +++ b/src/__tests__/ConfirmationDialog.test.tsx @@ -15,7 +15,7 @@ import {behavesAsComponent, checkExports} from '../utils/testing' expect.extend(toHaveNoViolations) -const Basic = ({autoFocusButton = 'confirm'}: {autoFocusButton?: 'confirm' | 'cancel'}) => { +const Basic = ({confirmButtonType}: Pick, 'confirmButtonType'>) => { const [isOpen, setIsOpen] = useState(false) const buttonRef = useRef(null) const onDialogClose = useCallback(() => setIsOpen(false), []) @@ -32,7 +32,7 @@ const Basic = ({autoFocusButton = 'confirm'}: {autoFocusButton?: 'confirm' | 'ca onClose={onDialogClose} cancelButtonContent="Secondary" confirmButtonContent="Primary" - autoFocusButton={autoFocusButton} + confirmButtonType={confirmButtonType} > Lorem ipsum dolor sit Pippin good dog. @@ -96,7 +96,7 @@ describe('ConfirmationDialog', () => { cleanup() }) - it('focuses the primary action when opened', async () => { + it('focuses the primary action when opened and the confirmButtonType is not set', async () => { const {getByText} = HTMLRender() act(() => { fireEvent.click(getByText('Show dialog')) @@ -106,8 +106,8 @@ describe('ConfirmationDialog', () => { cleanup() }) - it('focuses the primary action when opened with autoFocusButton set to confirm', async () => { - const {getByText} = HTMLRender() + it('focuses the primary action when opened and the confirmButtonType is not danger', async () => { + const {getByText} = HTMLRender() act(() => { fireEvent.click(getByText('Show dialog')) }) @@ -116,8 +116,8 @@ describe('ConfirmationDialog', () => { cleanup() }) - it('focuses the secondary action when opened with autoFocusButton set to cancel', async () => { - const {getByText} = HTMLRender() + it('focuses the secondary action when opened and the confirmButtonType is danger', async () => { + const {getByText} = HTMLRender() act(() => { fireEvent.click(getByText('Show dialog')) }) From e0165085fa6fb50fb8d8f548790bdd9393996bfe Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 26 Jul 2022 14:24:24 +0000 Subject: [PATCH 4/4] intermediate value --- src/Dialog/ConfirmationDialog.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Dialog/ConfirmationDialog.tsx b/src/Dialog/ConfirmationDialog.tsx index 59a3efe9cf3..98fcebdb390 100644 --- a/src/Dialog/ConfirmationDialog.tsx +++ b/src/Dialog/ConfirmationDialog.tsx @@ -116,17 +116,17 @@ export const ConfirmationDialog: React.FC = props => { const onConfirmButtonClick = useCallback(() => { onClose('confirm') }, [onClose]) - + const isConfirmationDangerous = confirmButtonType === 'danger' const cancelButton: DialogButtonProps = { content: cancelButtonContent, onClick: onCancelButtonClick, - autoFocus: confirmButtonType === 'danger' + autoFocus: isConfirmationDangerous } const confirmButton: DialogButtonProps = { content: confirmButtonContent, buttonType: confirmButtonType, onClick: onConfirmButtonClick, - autoFocus: confirmButtonType !== 'danger' + autoFocus: !isConfirmationDangerous } const footerButtons = [cancelButton, confirmButton] return (