Fix & refactor popover positioning#68026
Conversation
refactor Popovers to (mostly, use same logic for popover anchor)
blazejkustra
left a comment
There was a problem hiding this comment.
Solid refactor, looks much cleaner!
| vertical: 72, | ||
| }, | ||
| POPOVER_DROPDOWN_WIDTH: 336, | ||
| POPOVER_DROPDOWN_WIDTH: 334, |
There was a problem hiding this comment.
Why was this change necessary?
There was a problem hiding this comment.
334 is how it render width after size calculation. Without this change there is 2 px width flicker on the first render.
|
@jmusial FYI Tests are failing 👀 |
…ination of actions with side panel
src/pages/settings/Subscription/CardSection/CardSectionActions/index.tsx
Show resolved
Hide resolved
blazejkustra
left a comment
There was a problem hiding this comment.
Looks good to me, let's test it extensively (both popovers and help panel).
Also, can we wait with making this PR 'ready for review' for now? I would prefer to have this merged first 🙏
| setIsMenuVisible(false); | ||
| } | ||
| }} | ||
| anchorPosition={shouldUseStyleUtilityForAnchorPosition ? styles.popoverButtonDropdownMenuOffset(windowWidth) : popoverAnchorPosition} |
There was a problem hiding this comment.
Should we also clean up popoverButtonDropdownMenuOffset?
|
Back to you @hoangzinh |
|
I found a bug: Bug 1: Unable to add expense or export expense Screen.Recording.2025-08-29.at.05.52.31.mov |
|
The video is going through many screens and popovers, do I understand correctly that the only issue in this video is unclickable 'Add expense' at the start? @hoangzinh
|
|
@blazejkustra Ah yes, it's "unclickable 'Add expense' at the start". I sent a not-so-good recording 🤦 |
|
Great catch, there was an infinite loop as anchor default value was initialized with every render. Should work now @hoangzinh! |
trjExpensify
left a comment
There was a problem hiding this comment.
No concerns with what this is doing in the product. For the foreseeable future while we focus on #migrate, however, I don’t think we should be prioritising help panel issues as it's behind a beta NVP flag - and don't plan to release it anytime soon. CC: @JmillsExpensify
According to the PR's description, @blazejkustra, can you list some of them? I found that we already removed |
|
@hoangzinh I think PR description is outdated and I can't edit it 😢 |
No worry @blazejkustra. Can you share an updated version (just summary) of it here? |
|
@hoangzinh I mean this part is outdated:
Meaning I don't think there is anything to list 😄 But we can wait for @jmusial to confirm this. |
|
Thanks @blazejkustra |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #66244 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 9.2.1-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.1-20 🚀
|
| const prevAnchorPosition = usePrevious(anchorPosition); | ||
| const prevWindowDimensions = usePrevious({windowWidth, windowHeight}); | ||
| const {shouldHideSidePanel} = useSidePanel(); | ||
| const availableWidth = windowWidth - (shouldHideSidePanel ? 0 : variables.sideBarWidth); |

Explanation of Change
Problems
Solutions
Note
Not all popovers were migrated to new calculation, but the PR was growing too much. In the leftover cases it makes more sense to use 'legacy' calculation method, but would be good to go through them as a follow up and migrate as well.
Fixed Issues
$ #66244
PROPOSAL:
N/A
Tests
At no points during tests popover should overlap with Help panel.
Changes should affect only wide screen envorinments, since on narrow screens popovers are replaced with Bottom Docked Modals.
1. Button With Dropdown Menu
2. Dropdown Button
3. Chat avatar
4. Delegated access & Copilot
5. Workspaces list
Offline tests
Same as tests
QA Steps
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))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
Popovers are not used on narrow screens.
Android: mWeb Chrome
Popovers are not used on narrow screens.
iOS: Native
Popovers are not used on narrow screens.
iOS: mWeb Safari
Popovers are not used on narrow screens.
MacOS: Chrome / Safari
0038.desktop.chrome.mov
MacOS: Desktop
0038.desktop.native.mov