Simpler nav guard for SideModalForm (#2328)

* Simpler nav guard for SideModalForm * fix issue with isDirty evaluation * Don't trigger guard when clicking normal cancel button on form * Add test * a little cleanup * inline some stuff * inline the entire nav guard modal * tweak the test. we love to test * make confirm nav on side modal and full page forms a little narrower * fix double overlay by turning off overlay for side modal confirm only --------- Co-authored-by: David Crespo <[email protected]>
oxidecomputer · Nov 19, 2024 · 6fe6936 · 6fe6936
1 parent fcd3441
commit 6fe6936
Show file tree

Hide file tree

Showing 5 changed files with 108 additions and 15 deletions.
diff --git a/.eslintrc.cjs b/.eslintrc.cjs
@@ -118,6 +118,7 @@ module.exports = {
           'warn',
           { assertFunctionNames: ['expectVisible', 'expectRowVisible', 'expectOptions'] },
         ],
+        'playwright/no-force-option': 'off',
       },
     },
   ],

diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx
@@ -101,7 +101,7 @@ export function FullPageForm<TFieldValues extends FieldValues>({
       </form>
 
       {/* 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 && <ConfirmNavigation blocker={blocker} />}
 
@@ -126,10 +126,12 @@ const ConfirmNavigation = ({ blocker }: { blocker: Blocker }) => (
     isOpen={blocker.state === 'blocked'}
     onDismiss={() => blocker.reset?.()}
     title="Confirm navigation"
+    narrow
   >
     <Modal.Section>
-      Are you sure you want to leave this page? <br /> You will lose all progress on this
-      form.
+      Are you sure you want to leave this page?
+      <br />
+      All progress will be lost.
     </Modal.Section>
     <Modal.Footer
       onDismiss={() => blocker.reset?.()}

diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx
@@ -5,13 +5,14 @@
  *
  * 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'
 
 import type { ApiError } from '@oxide/api'
 
 import { Button } from '~/ui/lib/Button'
+import { Modal } from '~/ui/lib/Modal'
 import { SideModal } from '~/ui/lib/SideModal'
 
 type CreateFormProps = {
@@ -80,7 +81,6 @@ export function SideModalForm<TFieldValues extends FieldValues>({
   subtitle,
 }: SideModalFormProps<TFieldValues>) {
   const id = useId()
-  const { isSubmitting } = form.formState
 
   useEffect(() => {
     if (submitError?.errorCode === 'ObjectAlreadyExists' && 'name' in form.getValues()) {
@@ -94,9 +94,14 @@ export function SideModalForm<TFieldValues extends FieldValues>({
       ? `Update ${resourceName}`
       : submitLabel || title || `Create ${resourceName}`
 
+  // 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)
+
   return (
     <SideModal
-      onDismiss={onDismiss}
+      onDismiss={() => (isDirty ? setShowNavGuard(true) : onDismiss())}
       isOpen
       title={title || `${formType === 'edit' ? 'Edit' : 'Create'} ${resourceName}`}
       animate={useShouldAnimateModal()}
@@ -139,6 +144,29 @@ export function SideModalForm<TFieldValues extends FieldValues>({
           </Button>
         )}
       </SideModal.Footer>
+
+      {showNavGuard && (
+        <Modal
+          isOpen
+          onDismiss={() => setShowNavGuard(false)}
+          title="Confirm navigation"
+          narrow
+          overlay={false}
+        >
+          <Modal.Section>
+            Are you sure you want to leave this form?
+            <br />
+            All progress will be lost.
+          </Modal.Section>
+          <Modal.Footer
+            onAction={onDismiss}
+            onDismiss={() => setShowNavGuard(false)}
+            cancelText="Keep editing"
+            actionText="Leave form"
+            actionType="danger"
+          />
+        </Modal>
+      )}
     </SideModal>
   )
 }
diff --git a/app/ui/lib/Modal.tsx b/app/ui/lib/Modal.tsx
@@ -7,6 +7,7 @@
  */
 import * as Dialog from '@radix-ui/react-dialog'
 import { animated, useTransition } from '@react-spring/web'
+import cn from 'classnames'
 import React, { forwardRef, useId } from 'react'
 
 import { Close12Icon } from '@oxide/design-system/icons/react'
@@ -18,17 +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
+  /** 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 }: ModalProps) {
+export function Modal({
+  children,
+  onDismiss,
+  title,
+  isOpen,
+  narrow,
+  overlay = true,
+}: ModalProps) {
   const titleId = useId()
   const AnimatedDialogContent = animated(Dialog.Content)
 
@@ -54,9 +66,13 @@ export function Modal({ children, onDismiss, title, isOpen }: ModalProps) {
               modal={false}
             >
               <Dialog.Portal>
-                <DialogOverlay />
+                {overlay && <DialogOverlay />}
+
                 <AnimatedDialogContent
-                  className="pointer-events-auto fixed left-1/2 top-1/2 z-modal m-0 flex max-h-[min(800px,80vh)] w-auto min-w-[28rem] max-w-[32rem] flex-col justify-between rounded-lg border p-0 bg-raise border-secondary elevation-2"
+                  className={cn(
+                    'pointer-events-auto fixed left-1/2 top-1/2 z-modal m-0 flex max-h-[min(800px,80vh)] w-auto min-w-[24rem] flex-col justify-between rounded-lg border p-0 bg-raise border-secondary elevation-2',
+                    narrow ? 'max-w-[24rem]' : 'max-w-[32rem]'
+                  )}
                   aria-labelledby={titleId}
                   style={{
                     transform: y.to((value) => `translate3d(-50%, ${-50 + value}%, 0px)`),
@@ -68,11 +84,9 @@ export function Modal({ children, onDismiss, title, isOpen }: ModalProps) {
                   // https://github.com/oxidecomputer/console/issues/1745
                   onFocusOutside={(e) => e.preventDefault()}
                 >
-                  {title && (
-                    <Dialog.Title asChild>
-                      <ModalTitle id={titleId}>{title}</ModalTitle>
-                    </Dialog.Title>
-                  )}
+                  <Dialog.Title asChild>
+                    <ModalTitle id={titleId}>{title}</ModalTitle>
+                  </Dialog.Title>
                   {children}
                   <Dialog.Close
                     className="absolute right-2 top-3 flex rounded p-2 hover:bg-hover"

diff --git a/test/e2e/nav-guard-modal.e2e.ts b/test/e2e/nav-guard-modal.e2e.ts
@@ -0,0 +1,48 @@
+/*
+ * 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, expectObscured, test } from './utils'
+
+test('navigating away from SideModal form triggers nav guard', async ({ page }) => {
+  const floatingIpsPage = '/projects/mock-project/floating-ips'
+  const formModal = page.getByRole('dialog', { name: 'Create floating IP' })
+  const confirmModal = page.getByRole('dialog', { name: 'Confirm navigation' })
+
+  await page.goto(floatingIpsPage)
+
+  // 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
+
+  // 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 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
+  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)
+})
-Original file line number
+Diff line change
@@ Expand Up / @@ -118,6 +118,7 @@ module.exports = { @@
               'warn',
               { assertFunctionNames: ['expectVisible', 'expectRowVisible', 'expectOptions'] },
             ],
+            'playwright/no-force-option': 'off',
           },
         },
       ],
@@ Expand Down @@