[No-QA] refactor: introduce shared Overlay primitives#94127
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!
|
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.
|
@TaduJR This PR introduce ~4000 lines of new code have no production consumer — nothing in Modal, Popover, or PopoverMenu imports from it. The unit tests verify internal logic, but real integration (focus, navigation, Onyx state) is untested until M2-M5. Could we include at least one thin integration proof (e.g., migrating a single popover to FloatingHost) to validate the foundation end-to-end before building on top of it? cc @roryabraham, what are your thoughts? |
|
I agree that integrating at least one usage so we have something to test would be ideal |
Took sometime and written README. I think it deserves README.md like we did on PopoverMenu. |
|
Hi Everyone, How are we doing here? I see that blocker PR is merged. |
|
I'll continue the review today. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
This is a new pattern for storing data, similar to a tiny Zustand-style store. @roryabraham, what do you think about this pattern?
There was a problem hiding this comment.
without digging in, my sense is that we almost certainly don't need it. It seems relatively reasonable, but we shouldn't introduce new patterns lightly because they have a tendency to spread virally throughout the code. This is definitely one I'd want to discuss in #expert-contributors with a clear understanding for why we're proposing adding it.
There was a problem hiding this comment.
Removed and made it inline.
But we should really handle this in appropriate and DRY way because I am seeing similar pattern on over ~10 places.
I think discussion with #expert-contributor is a great idea!
There was a problem hiding this comment.
Can you list out some of the places where you see a similar pattern being used?
There was a problem hiding this comment.
Sure
src/libs/NetworkState.ts(viauseNetwork) offline/reconnect statesrc/libs/RevealedCardSecretsStore.tsalready a small factory (createRevealedSecretStore<T>), 2 instances (card PIN + virtual-card details)src/components/Navigation/SearchSidebarCollapseStore.tssidebar peek statesrc/libs/AgentZeroReasoningStore.tsandsrc/libs/AgentZeroOptimisticStore.ts(both viauseAgentZeroStatusIndicator) keyed reasoning/optimistic storessrc/components/Charts/utils/chartFontsCache.ts(viauseChartFonts) loaded-fonts cachesrc/components/Overlay/libs/dismissableLayerStore.ts+overlayStore.tsthis PR
After more depth look the other 4 are bridging an external event source rather than an in-memory store (so a store helper wouldn't apply) imo
…erlay-foundation
…erlay-foundation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70878ba224
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…rs default export
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28f76abb51
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
This comment was marked as resolved.
This comment was marked as resolved.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ 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". |
…erlay-foundation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b731e8c2d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274b2a518d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65b414b41d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| }; | ||
|
|
||
| function FloatingHost({isOpen, anchor, anchorRect, alignment, offsetPx, fadeDuration, onDismiss, onExitComplete, surfaceStyle, stackId, containFocus = false, children}: FloatingHostProps) { | ||
| const {style: positionStyle, available, isPositioned, onContentLayout} = useAnchoredPosition({anchorRect, alignment, offsetPx}); |
There was a problem hiding this comment.
Preserve the anchor while the popover exits
When consumers follow the documented pattern and clear the popover state in onDismiss, isOpen becomes false and anchorRect becomes null while Presence still keeps the surface mounted for the exit animation. Passing that live anchorRect into useAnchoredPosition makes it return the hidden top-left style (opacity: 0), so the parent wrapper hides/jumps the popover before AnimatedSurface can visibly fade it out. Keep the last measured anchor/position through the unmount-suspended phase, or otherwise avoid applying the measurement gate during exit.
Useful? React with 👍 / 👎.
Explanation of Change
M1 of a five-PR split of the Modal v2 / Dialog / Popover v2 / PopoverMenu v2 decomposition. This PR ships the shared bottom layer — a new
Overlay/primitive folder plus a few small cross-feature hooks/libs — that the next four PRs compose on. No consumer migration here; every existing user-facing surface is byte-identical tomain.Contents:
src/components/Overlay/:DismissableLayer,FloatingHost,Portal,Presence,AnimatedSurface,createHeadingSystem, plus 12 hooks (Esc handling, anchored positioning, aria-hide, body-scroll lock, focus management, etc.) and 7 libs (anchor measurement, dismissable-layer stack store, external-store factory, etc.)useCallbackRef,useControlledState,useDisclosureStatecomposeEventHandlers,isHTMLElementFocusTrapForModal.shouldReturnFocusaccepts() => booleanso M2 can resolve the Composer-focus baton at restore timePressableRefElementfromPressable/GenericPressable/types.tsso M1'smeasureAnchor/overlayStorecan type their anchor refsOVERLAY_FADE_IN_WEB/OVERLAY_FADE_OUT_WEB), 1 style (overlayPortalHost)Fixed Issues
$ #91238
PROPOSAL:
Tests
Offline 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