From 577be18db009f25169694e7f43f74f79479da232 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 17 Jul 2024 10:38:10 -0400 Subject: [PATCH 01/10] Simpler nav guard for SideModalForm --- app/components/form/NavGuardModal.tsx | 31 +++++++++++++++++++++++++++ app/components/form/SideModalForm.tsx | 15 ++++++++++--- 2 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 app/components/form/NavGuardModal.tsx diff --git a/app/components/form/NavGuardModal.tsx b/app/components/form/NavGuardModal.tsx new file mode 100644 index 000000000..5012b4af3 --- /dev/null +++ b/app/components/form/NavGuardModal.tsx @@ -0,0 +1,31 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import { Modal } from '~/ui/lib/Modal' + +const NavGuardModal = ({ + onAction, + onDismiss, +}: { + onAction: () => void + onDismiss: () => void +}) => ( + + + Are you sure you want to leave this form? Your progress will be lost. + + + +) + +export { NavGuardModal } diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index e0e02b7cb..bc5e814d3 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { useEffect, useId, type ReactNode } from 'react' +import { useEffect, useId, useState, type ReactNode } from 'react' import type { FieldValues, UseFormReturn } from 'react-hook-form' import { NavigationType, useNavigationType } from 'react-router-dom' @@ -14,6 +14,8 @@ import type { ApiError } from '@oxide/api' import { Button } from '~/ui/lib/Button' import { SideModal } from '~/ui/lib/SideModal' +import { NavGuardModal } from './NavGuardModal' + type CreateFormProps = { formType: 'create' /** Only needed if you need to override the default button text (`Create ${resourceName}`) */ @@ -89,9 +91,13 @@ export function SideModalForm({ ? `Update ${resourceName}` : submitLabel || title || `Create ${resourceName}` + const [showNavGuard, setShowNavGuard] = useState(false) + const guardedDismiss = () => + form.formState.isDirty ? setShowNavGuard(true) : onDismiss() + return ( ({ - {onSubmit && ( @@ -134,6 +140,9 @@ export function SideModalForm({ )} + {showNavGuard && ( + setShowNavGuard(false)} onAction={onDismiss} /> + )} ) } From cb5486a86ef066f5b28702cb58d0ea1262322d3a Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 17 Jul 2024 10:56:02 -0400 Subject: [PATCH 02/10] fix issue with isDirty evaluation --- app/components/form/SideModalForm.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index bc5e814d3..84b699da4 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -91,9 +91,9 @@ export function SideModalForm({ ? `Update ${resourceName}` : submitLabel || title || `Create ${resourceName}` + const { isDirty } = form.formState const [showNavGuard, setShowNavGuard] = useState(false) - const guardedDismiss = () => - form.formState.isDirty ? setShowNavGuard(true) : onDismiss() + const guardedDismiss = () => (isDirty ? setShowNavGuard(true) : onDismiss()) return ( Date: Wed, 17 Jul 2024 10:57:21 -0400 Subject: [PATCH 03/10] Don't trigger guard when clicking normal cancel button on form --- app/components/form/SideModalForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index 84b699da4..07c0d6692 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -124,7 +124,7 @@ export function SideModalForm({ - {onSubmit && ( From 5e3401542b6a83e5c83a2cc9865de362512ea8ed Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 17 Jul 2024 11:14:45 -0400 Subject: [PATCH 04/10] Add test --- .eslintrc.cjs | 1 + test/e2e/nav-guard-modal.e2e.ts | 47 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 test/e2e/nav-guard-modal.e2e.ts diff --git a/.eslintrc.cjs b/.eslintrc.cjs index a212a7b04..8df277691 100644 --- a/.eslintrc.cjs +++ b/.eslintrc.cjs @@ -113,6 +113,7 @@ module.exports = { 'warn', { assertFunctionNames: ['expectVisible', 'expectRowVisible'] }, ], + 'playwright/no-force-option': 'off', }, }, ], diff --git a/test/e2e/nav-guard-modal.e2e.ts b/test/e2e/nav-guard-modal.e2e.ts new file mode 100644 index 000000000..4e27dbe52 --- /dev/null +++ b/test/e2e/nav-guard-modal.e2e.ts @@ -0,0 +1,47 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ + +import { expect, expectVisible, test } from './utils' + +test('navigating away from SideModal form triggers nav guard', async ({ page }) => { + const floatingIpsPage = '/projects/mock-project/floating-ips' + const floatingIpName = 'my-floating-ip' + const formModal = page.getByRole('dialog', { name: 'Create floating IP' }) + const confirmModal = page.getByRole('dialog', { name: 'Confirm navigation' }) + + await page.goto(floatingIpsPage) + await page.locator('text="New Floating IP"').click() + + await expectVisible(page, [ + 'role=heading[name*="Create floating IP"]', + 'role=textbox[name="Name"]', + 'role=textbox[name="Description"]', + 'role=button[name="Advanced"]', + 'role=button[name="Create floating IP"]', + ]) + + await page.fill('input[name=name]', floatingIpName) + + // form is now dirty, so clicking away should trigger the nav guard + // force: true allows us to click even though the "Instances" link is inactive + await page.getByRole('link', { name: 'Instances' }).click({ force: true }) + await expect(confirmModal).toBeVisible() + + // go back to the form + await page.getByRole('button', { name: 'Keep editing' }).click() + await expect(confirmModal).toBeHidden() + await expect(formModal).toBeVisible() + + // now try to navigate away again; verify that clicking the Escape key also triggers it + await page.keyboard.press('Escape') + await expect(confirmModal).toBeVisible() + await page.getByRole('button', { name: 'Leave form' }).click() + await expect(confirmModal).toBeHidden() + await expect(formModal).toBeHidden() + await expect(page).toHaveURL(floatingIpsPage) +}) From a90c0056c2356fa2879aadd9e1550e450c27deb5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 14 Nov 2024 13:31:29 -0800 Subject: [PATCH 05/10] a little cleanup --- app/components/form/NavGuardModal.tsx | 9 ++------- app/components/form/SideModalForm.tsx | 3 +-- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/app/components/form/NavGuardModal.tsx b/app/components/form/NavGuardModal.tsx index 5012b4af3..b94b29c16 100644 --- a/app/components/form/NavGuardModal.tsx +++ b/app/components/form/NavGuardModal.tsx @@ -7,13 +7,8 @@ */ import { Modal } from '~/ui/lib/Modal' -const NavGuardModal = ({ - onAction, - onDismiss, -}: { - onAction: () => void - onDismiss: () => void -}) => ( +type NavGuardModalProps = { onAction: () => void; onDismiss: () => void } +const NavGuardModal = ({ onAction, onDismiss }: NavGuardModalProps) => ( Are you sure you want to leave this form? Your progress will be lost. diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index 4a83bf45e..a08927383 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -11,11 +11,10 @@ import { NavigationType, useNavigationType } from 'react-router-dom' import type { ApiError } from '@oxide/api' +import { NavGuardModal } from '~/components/form/NavGuardModal' import { Button } from '~/ui/lib/Button' import { SideModal } from '~/ui/lib/SideModal' -import { NavGuardModal } from './NavGuardModal' - type CreateFormProps = { formType: 'create' /** Only needed if you need to override the default button text (`Create ${resourceName}`) */ From 57b364d45f3a4fe8992e54b5e758bda15ddcf25e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 19 Nov 2024 09:04:32 -0600 Subject: [PATCH 06/10] inline some stuff --- app/components/form/SideModalForm.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index a08927383..7f01fed85 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -81,7 +81,6 @@ export function SideModalForm({ subtitle, }: SideModalFormProps) { const id = useId() - const { isSubmitting } = form.formState useEffect(() => { if (submitError?.errorCode === 'ObjectAlreadyExists' && 'name' in form.getValues()) { @@ -95,13 +94,14 @@ export function SideModalForm({ ? `Update ${resourceName}` : submitLabel || title || `Create ${resourceName}` - const { isDirty } = form.formState + // must be destructured up here to subscribe to changes. inlining + // form.formState.isDirty does not work + const { isDirty, isSubmitting } = form.formState const [showNavGuard, setShowNavGuard] = useState(false) - const guardedDismiss = () => (isDirty ? setShowNavGuard(true) : onDismiss()) return ( (isDirty ? setShowNavGuard(true) : onDismiss())} isOpen title={title || `${formType === 'edit' ? 'Edit' : 'Create'} ${resourceName}`} animate={useShouldAnimateModal()} From 9d8513a4025da96e5dd5b8ac9e1fc081552ef1d5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 19 Nov 2024 09:12:32 -0600 Subject: [PATCH 07/10] inline the entire nav guard modal --- app/components/form/NavGuardModal.tsx | 26 -------------------------- app/components/form/SideModalForm.tsx | 16 ++++++++++++++-- 2 files changed, 14 insertions(+), 28 deletions(-) delete mode 100644 app/components/form/NavGuardModal.tsx diff --git a/app/components/form/NavGuardModal.tsx b/app/components/form/NavGuardModal.tsx deleted file mode 100644 index b94b29c16..000000000 --- a/app/components/form/NavGuardModal.tsx +++ /dev/null @@ -1,26 +0,0 @@ -/* - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * Copyright Oxide Computer Company - */ -import { Modal } from '~/ui/lib/Modal' - -type NavGuardModalProps = { onAction: () => void; onDismiss: () => void } -const NavGuardModal = ({ onAction, onDismiss }: NavGuardModalProps) => ( - - - Are you sure you want to leave this form? Your progress will be lost. - - - -) - -export { NavGuardModal } diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index 7f01fed85..076a5068a 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -11,8 +11,8 @@ import { NavigationType, useNavigationType } from 'react-router-dom' import type { ApiError } from '@oxide/api' -import { NavGuardModal } from '~/components/form/NavGuardModal' import { Button } from '~/ui/lib/Button' +import { Modal } from '~/ui/lib/Modal' import { SideModal } from '~/ui/lib/SideModal' type CreateFormProps = { @@ -144,8 +144,20 @@ export function SideModalForm({ )} + {showNavGuard && ( - setShowNavGuard(false)} onAction={onDismiss} /> + setShowNavGuard(false)} title="Confirm navigation"> + + Are you sure you want to leave this form? Your progress will be lost. + + setShowNavGuard(false)} + cancelText="Keep editing" + actionText="Leave form" + actionType="danger" + /> + )} ) From 3a52f0a6d7584173edc7ef5439a568b856e501a3 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 19 Nov 2024 09:24:36 -0600 Subject: [PATCH 08/10] tweak the test. we love to test --- test/e2e/nav-guard-modal.e2e.ts | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/test/e2e/nav-guard-modal.e2e.ts b/test/e2e/nav-guard-modal.e2e.ts index 4e27dbe52..e3ef40c6f 100644 --- a/test/e2e/nav-guard-modal.e2e.ts +++ b/test/e2e/nav-guard-modal.e2e.ts @@ -6,30 +6,31 @@ * Copyright Oxide Computer Company */ -import { expect, expectVisible, test } from './utils' +import { expect, expectObscured, test } from './utils' test('navigating away from SideModal form triggers nav guard', async ({ page }) => { const floatingIpsPage = '/projects/mock-project/floating-ips' - const floatingIpName = 'my-floating-ip' const formModal = page.getByRole('dialog', { name: 'Create floating IP' }) const confirmModal = page.getByRole('dialog', { name: 'Confirm navigation' }) await page.goto(floatingIpsPage) - await page.locator('text="New Floating IP"').click() - await expectVisible(page, [ - 'role=heading[name*="Create floating IP"]', - 'role=textbox[name="Name"]', - 'role=textbox[name="Description"]', - 'role=button[name="Advanced"]', - 'role=button[name="Create floating IP"]', - ]) + // we don't have to force click here because it's not covered by the modal overlay yet + await expect(formModal).toBeHidden() + const somethingOnPage = page.getByRole('heading', { name: 'Floating IPs' }) + await somethingOnPage.click({ trial: true }) // test that it's not obscured - await page.fill('input[name=name]', floatingIpName) + // now open the modal + await page.getByRole('link', { name: 'New Floating IP' }).click() + await expectObscured(somethingOnPage) // it's covered by overlay + await expect(formModal).toBeVisible() + await formModal.getByRole('textbox', { name: 'Name' }).fill('my-floating-ip') // form is now dirty, so clicking away should trigger the nav guard - // force: true allows us to click even though the "Instances" link is inactive - await page.getByRole('link', { name: 'Instances' }).click({ force: true }) + // force: true allows us to click in that spot even though the thing is obscured + await expect(confirmModal).toBeHidden() + await somethingOnPage.click({ force: true }) + await expect(formModal).toBeVisible() await expect(confirmModal).toBeVisible() // go back to the form From 5e17f9b208d532f7f2fdd80b8a7e830931e5cf50 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 19 Nov 2024 11:50:47 -0600 Subject: [PATCH 09/10] make confirm nav on side modal and full page forms a little narrower --- app/components/form/FullPageForm.tsx | 8 +++++--- app/components/form/SideModalForm.tsx | 11 +++++++++-- app/ui/lib/Modal.tsx | 9 +++++++-- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx index 4ca6f79b9..fe422f543 100644 --- a/app/components/form/FullPageForm.tsx +++ b/app/components/form/FullPageForm.tsx @@ -101,7 +101,7 @@ export function FullPageForm({ {/* rendering of the modal must be gated on isSubmitSuccessful because - there is a brief moment where isSubmitSuccessful is true but the proceed() + there is a brief moment where isSubmitSuccessful is true but the proceed() hasn't fired yet, which means we get a brief flash of this modal */} {!isSubmitSuccessful && } @@ -126,10 +126,12 @@ const ConfirmNavigation = ({ blocker }: { blocker: Blocker }) => ( isOpen={blocker.state === 'blocked'} onDismiss={() => blocker.reset?.()} title="Confirm navigation" + narrow > - Are you sure you want to leave this page?
You will lose all progress on this - form. + Are you sure you want to leave this page? +
+ All progress will be lost. blocker.reset?.()} diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index 076a5068a..b158bfe63 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -146,9 +146,16 @@ export function SideModalForm({ {showNavGuard && ( - setShowNavGuard(false)} title="Confirm navigation"> + setShowNavGuard(false)} + title="Confirm navigation" + narrow + > - Are you sure you want to leave this form? Your progress will be lost. + Are you sure you want to leave this form? +
+ All progress will be lost. void + narrow?: boolean } // Note that the overlay has z-index 30 and content has 40. This is to make sure // both land on top of a side modal in the regrettable case where we have both // on screen at once. -export function Modal({ children, onDismiss, title, isOpen }: ModalProps) { +export function Modal({ children, onDismiss, title, isOpen, narrow }: ModalProps) { const titleId = useId() const AnimatedDialogContent = animated(Dialog.Content) @@ -56,7 +58,10 @@ export function Modal({ children, onDismiss, title, isOpen }: ModalProps) { `translate3d(-50%, ${-50 + value}%, 0px)`), From 80b49a3583fb5a5f48ed149e8f58aeec3db99962 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 19 Nov 2024 12:02:53 -0600 Subject: [PATCH 10/10] fix double overlay by turning off overlay for side modal confirm only --- app/components/form/SideModalForm.tsx | 1 + app/ui/lib/Modal.tsx | 27 ++++++++++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index b158bfe63..d1e6075fe 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -151,6 +151,7 @@ export function SideModalForm({ onDismiss={() => setShowNavGuard(false)} title="Confirm navigation" narrow + overlay={false} > Are you sure you want to leave this form? diff --git a/app/ui/lib/Modal.tsx b/app/ui/lib/Modal.tsx index ac244b04d..d6877b7d7 100644 --- a/app/ui/lib/Modal.tsx +++ b/app/ui/lib/Modal.tsx @@ -19,18 +19,28 @@ import { DialogOverlay } from './DialogOverlay' import { ModalContext } from './modal-context' export type ModalProps = { - title?: string + title: string isOpen: boolean children?: React.ReactNode onDismiss: () => void - narrow?: boolean + /** Default false. Only needed in a couple of spots. */ + narrow?: true + /** Default true. We only need to hide it for the rare case of modal on top of modal. */ + overlay?: boolean } // Note that the overlay has z-index 30 and content has 40. This is to make sure // both land on top of a side modal in the regrettable case where we have both // on screen at once. -export function Modal({ children, onDismiss, title, isOpen, narrow }: ModalProps) { +export function Modal({ + children, + onDismiss, + title, + isOpen, + narrow, + overlay = true, +}: ModalProps) { const titleId = useId() const AnimatedDialogContent = animated(Dialog.Content) @@ -56,7 +66,8 @@ export function Modal({ children, onDismiss, title, isOpen, narrow }: ModalProps modal={false} > - + {overlay && } + e.preventDefault()} > - {title && ( - - {title} - - )} + + {title} + {children}