refactor: Decompose Modal - #92881
Conversation
|
@linhvovan29546 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Sorry for the delay. This one was tough and need some time. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@TaduJR is there any way we can split this up into multiple smaller PRs? For just a few examples:
|
Yes it's possible @roryabraham.
This is a cosmetic one but I really recommend it. We can split it in to 4 PR's PR 1 - Sound good? |
Let's put it in its own PR then to keep this PR as small as can be. Seems trivial to land separately. Otherwise, the split you propose sounds good. |
b7a4ccb to
047d65a
Compare
|
|
Explanation of Change
This PR introduces a v2 Modal/Popover/PopoverMenu/Dialog system as Radix-inspired compound primitives, replacing the v1 boolean-prop and lifecycle-leak debt at the foundation layer. v1 paths are untouched — v2 ships at sibling paths; existing callers keep importing v1.
What ships
Overlay/— shared foundation:Presence,DismissableLayer,Portal,AnimatedSurface, observable stores, platform-split side-effect hooksModal/v2/— compound primitive + 6 variants (BottomDockedModal,CenteredModal,CenteredSmallModal,ConfirmDialog,FullscreenModal,RightDockedModal) + slot-basedconfirm//decision/sub-compoundsPopover/v2/— anchored floating-panel compound withuseAnchoredOpenershared trigger flowPopoverMenu/v2/— re-platformed onto the new foundationDialog/— v2 replacement forModal/Global:DialogProviderqueue + 4 presenters (Confirm,Decision,HoldMenu,HRSyncResults)useConfirmDialog,useDecisionDialog,useHoldMenuDialog,useHRSyncResultsDialogmirror v1 hook shapes for one-line caller migrationSample consumer migrations (one per surface)
ComparePlansModal→CenteredModal/CenteredSmallModalAccountSwitcher(offline + GPS confirms) →useConfirmDialog+DialogProvidermounted app-wideMoneyReportHeaderModals→useDecisionDialog+useHoldMenuDialogWorkspaceHRPage+WorkspaceMembersPage→useHRSyncResultsDialogFixed Issues
$ #91238
PROPOSAL:
Tests
1. Compare Plans Modal — Settings → Subscription
2. AccountSwitcher confirms
Setup: Account with a delegate relationship (you delegate to someone, or someone delegates to you).
2.1 — Offline confirm
2.2 — GPS trip-in-progress confirm
3. Money report Hold Menu + Decision dialogs
3.1 — Hold Menu (partial / full)
3.2 — Export Decision dialogs (offline / failed download)
4. HR Sync Results dialog
Setup: Corporate workspace with Gusto or TriNet connected, you're a workspace admin.
isFocused).Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari