Replace Radix Dialog/AlertDialog with Tamagui (#582) - #790
Draft
gaidheal1 wants to merge 1 commit into
Draft
Conversation
Ports Modal and AlertDialog off @radix-ui/react-dialog/react-alert-dialog onto Tamagui's own Dialog/AlertDialog, which reuse the same portal/overlay/ focus-trap/dismissable-layer machinery Radix uses under the hood, so both components keep their public prop shapes and a11y contract (focus trap, role="dialog"/"alertdialog", Escape-to-close, click-outside behavior, scroll lock) unchanged for all 12 consumer files. Two integration gaps needed explicit handling rather than relying on Tamagui's defaults, both verified against the real test suite: - Neither component renders a `Trigger`, so Tamagui's built-in triggerRef-based focus-restore-on-close always no-ops; a shared useReturnFocusOnClose hook captures the pre-open focus target during render (before FocusScope's own mount effect can steal it) and restores it via onCloseAutoFocus. - Tamagui's Dialog.Close/AlertDialog.Cancel/.Action compose their auto-close behavior onto an `onPress` prop, which the app's plain (non-forwardRef) Button never receives as a real onClick under asChild - replaced with plain Buttons and explicit onClick handlers, with Cancel's auto-focus reimplemented via id + onOpenAutoFocus since the same asChild/ref gap breaks Tamagui's internal cancelRef too. Also fixes a duplicate ARIA dialog landmark: Tamagui's Portal renders as a literal <dialog> HTML tag, which carries an implicit "dialog" role alongside Content's own explicit role - role="presentation" on both Portals collapses it back to one landmark. Test suites for Modal/AlertDialog and all 12 consumers now wrap renders in TamaguiProvider (matching the #580/#581 pattern) and pass unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #582 (base branch note below). Ports
ModalandAlertDialogoff@radix-ui/react-dialog/@radix-ui/react-alert-dialogonto Tamagui's ownDialog/AlertDialog, per #591's decision (full Tamagui) and #578's epic. This is based onclaude/issue-581-toast-tamaguirather thandevelopment, per the requester's instruction to branch off it (and the issue's own note to sequence #582 after #580/#581).@radix-ui/react-alert-dialogimport remains anywhere, and the package is removed frompackage.json/lockfile.@radix-ui/react-dialogstill remains as a dependency - it's still used byDetailSurface.tsx(the map entity detail card), which is explicitly out of scope: its own docstring says this exact primitive swap is meant to happen in a separate step ("once the project's in-progress Radix → Tamagui migration reaches this component"), and it isn't one of the 5 consumer files Replace Radix Dialog and AlertDialog with a shared overlay primitive #582 lists. Converting it here would expand scope into Map-related code the issue doesn't mention.ModalandAlertDialogare unchanged; all consumers (12 files, more than the issue's original 5 -PlayerItemList,SupportFlowModal,TutorialModal,NotesPanel,LogOfflineActivityModalvia Modal;ActivityInput,UnifiedTimerHomevia AlertDialog; plusTasksPanel/ActivitiesPanel/ProjectsPanel/SkillsPanel/CategoriesPaneltransitively viaPlayerItemList) work untouched.Tamagui's
Dialog/AlertDialogturned out to be a much closer match to Radix's own API than the earlier PoC (.claude/plans/issue-629-tamagui-poc.md, which only coveredTooltip/Button) suggested was likely - it reuses hand-builtFocusScope/Dismissable/RemoveScroll/Portalpackages (not@tamagui/popper, which is what had Tooltip's confirmed focus bug), and exposes the sametrapFocus/onOpenAutoFocus/onCloseAutoFocus/onEscapeKeyDown/onPointerDownOutsidesurface Radix'sContentdoes.AlertDialog.Contentalso has Radix's exact "prevent click-outside" and "default-focus-Cancel" behavior baked in already.Two integration gaps still needed explicit handling, both found empirically (via actual failing tests, not guessed) rather than assumed from the API surface:
Dialog.Trigger(Modalis always-open/consumer-mounted,AlertDialogis driven by a controlledopenprop) - Tamagui's built-in close-focus-restore istriggerRef.current?.focus(), which always no-ops here, same as it silently did under Radix. Added a shareduseReturnFocusOnClosehook (src/components/Overlay/useReturnFocusOnClose.ts) that captures whatever had focus right before the overlay opened - read during render (React's documented "adjust state from a changed prop" pattern, not a ref mutation - this repo'sreact-hooks/refslint rule rejects the ref-mutation version) so it runs beforeFocusScope's own mount effect could steal focus first - and returns anonCloseAutoFocushandler that restores it. Covered by new tests in bothModal.test.tsxandAlertDialog.test.tsx.asChild+ the app's plainButton. Tamagui'sDialog.Close/AlertDialog.Cancel/.Actioncompose their click-to-close behavior onto anonPressprop, which the app'sButton(a plain function component, not RN-style, noforwardRef) never receives as a real DOMonClick- confirmed via a React "unknown event handleronPress" console warning and a failing click test. Replaced with plainButtons and explicitonClickhandlers instead of relying onasChild's composed behavior. Same root cause (noforwardRef) breaksAlertDialog's built-incancelRef-based auto-focus-on-open, so that's reimplemented via an elementid+onOpenAutoFocusinstead.Also found and fixed a duplicate ARIA landmark:
Dialog.Portal/AlertDialog.Portalrender as a literal<dialog>HTML tag, which carries an implicitdialogrole from the browser, stacking withContent's own explicitrole="dialog"/"alertdialog"-getByRole('dialog')was finding two elements. Fixed withrole="presentation"on both Portals.Acceptance criteria status
@radix-ui/react-dialog/@radix-ui/react-alert-dialogimport inModal/AlertDialog(package fully removed for alert-dialog;react-dialogstays installed for the out-of-scopeDetailSurface)FocusScope,trapFocus={context.open}) and restored to the triggering element on close (newuseReturnFocusOnClosehook, tested)role="dialog"vsrole="alertdialog"preserved, title/description associations intact (aria-labelledby/aria-describedbywired via Tamagui's own context IDs)Modalbut notAlertDialog(AlertDialog.Contentalready prevents this internally; new test asserts it)onOpenAutoFocus+ id, tested)RemoveScroll, unchanged from thecontext.open-gated default)Modal.test.tsx,AlertDialog.test.tsx,SupportFlowModal.test.tsx,TutorialModal.test.tsxall pass (plus the other 8 affected consumer suites)test:a11y- stories updated (stale Radix references fixed in docs/comments) and the globalTamaguiProviderdecorator in.storybook/preview.tsxalready covers them, but I could not actually run the Storybook/Playwright browser test projects in this sandbox - the pinned Playwright build (chromium_headless_shell-1234) isn't the version pre-installed here (-1194), a pre-existing environment gap unrelated to this diff (confirmed by reproducing the same failure against an unrelated story on the base branch). Please runnpm run test:a11y/npm run test:storybookin CI or locally to confirm.Test plan
npm run test(vitest run --project unit) - full suite green except one pre-existing, unrelated flake (UnifiedTimerHome > selecting a suggestion while unlabelled-running..., confirmed failing identically on the base branch before this change)npx tsc --noEmit- cleannpm run lint(eslint src) - cleannpm run build:production- builds successfullynpm run test:a11y/npm run test:storybook- blocked by the sandbox's Playwright browser version, see aboveGenerated by Claude Code