refactor(website): make confirmation dialog a global singleton#6789
Conversation
Replace the useConfirmDialog() hook (which returned { confirm, confirmDialog }
and required every call site to both call confirm() and render {confirmDialog})
with a module-level store and a single ConfirmDialogContainer host mounted once
in BaseLayout. Call sites now just import and call confirm() — no element to
render, and no risk of an early return omitting the dialog.
A module-level store rather than React context is required because each Astro
client:* island is an isolated React tree that context cannot cross. The host
renders in-tree (not a detached createRoot), so Headless UI still makes the
background inert and traps focus.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This PR may be related to: #6744 (Confirmation dialogs/modals should make background greyed out items inert for keyboard navigation) — the PR description notes that the new in-tree host means Headless UI still makes the background inert and traps focus, which directly addresses the accessibility concern raised in that issue. |
|
Very nice, tested locally and works 🎉 Just the tests now are unhappy: |
|
yep working on it, thanks! |
The confirmation dialog now renders via a single global host (in BaseLayout), so components calling displayConfirmation() no longer render the dialog themselves. Mount ConfirmDialogContainer alongside the component in the specs that exercise confirm flows (ReviewPage, SubmissionForm, FolderUploadComponent). Also reset the dialog store when the last host unsubscribes, so module state cannot leak between tests that remount the host. In the app the single host lives for the whole session, so this never fires there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
annoying amount of churn to supply this for the tests - I tried a global pattern where one wouldn't have needed changes for each test but that broke the hydrated button test. I think that possibly indicates that that test is overly fragile. Anyway, I'd be very keen to keep this single-function pattern one way or another. |
|
Not so worried about the test churn. The global state outside React isn't great but if you want the DX of single call I'm fine with it. |
|
This is the pattern we already have for toasts, and I do think it's significantly safer. Thanks for tolerating! |
2 participants
PR to Cornelius's PR #6780
Replace the useConfirmDialog() hook (which returned { confirm, confirmDialog } and required every call site to both call confirm() and render {confirmDialog}) with a module-level store and a single ConfirmDialogContainer host mounted once in BaseLayout. Call sites now just import and call confirm() — no element to render, and no risk of an early return omitting the dialog.
A module-level store rather than React context is required because each Astro client:* island is an isolated React tree that context cannot cross. The host renders in-tree (not a detached createRoot), so Headless UI still makes the background inert and traps focus.
🚀 Preview: https://confirmation.loculus.org