fix: Reports - More menu on expense RHP blocks primary and secondary button. #71162
Conversation
…button. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
melvin-bot
bot
requested review from
ikevin127
and removed request for
a team
|
@ikevin127 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] |
src/components/PopoverMenu.tsx
Outdated
| desktopMaxHeightStyle = {maxHeight: windowHeight - Math.round(top) - 20}; | ||
| } else { | ||
| desktopMaxHeightStyle = {maxHeight: windowHeight - 250}; | ||
| } |
There was a problem hiding this comment.
@ikevin127 should we add a maxHeight if the modal is not top anchored? Or should we leave it as it is?
There was a problem hiding this comment.
Yes, we should compute it symmetrically instead of using a static fallback, then have fallback for other anchor cases, here's a rewrite for the menuContainerStyle that ensures the following issues are addressed:
- Magic numbers: 20 and 250 are unexplained “magic numbers”, which violates clean code and typically Expensify’s style guidance (prefer constants or theme variables with intent-revealing names)
- Object spread for style merging vs style array: return
{...styles.createMenuContainer, ...desktopMaxHeightStyle}creates a new object and flattens style keys - Edge case: Negative maxHeight when top is near bottom: If top is close to windowHeight, windowHeight - Math.round(top) - 20 can be negative. On web, negative max-height is invalid and the rule is ignored; on native, it can result in unexpected layout. This regression would reintroduce the original bug (menu not constrained).
const menuContainerStyle = useMemo(() => {
const DEFAULT_MAX_HEIGHT_OFFSET = 250;
const SAFE_BOTTOM_SPACE = variables.h20;
if (!shouldEnableMaxHeight) {
return [styles.createMenuContainer];
}
if (isSmallScreenWidth) {
return [styles.createMenuContainer, {maxHeight: windowHeight - DEFAULT_MAX_HEIGHT_OFFSET}];
}
const isTopAnchored = anchorAlignment?.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP;
const top = anchorPosition?.vertical;
// Reserve space for footer actions when expanding downward.
if (isTopAnchored && typeof top === 'number') {
const computed = windowHeight - Math.round(top) - SAFE_BOTTOM_SPACE;
const maxHeight = Math.max(0, computed);
return [styles.createMenuContainer, {maxHeight}];
}
// Fallback for other anchor cases
return [styles.createMenuContainer, {maxHeight: windowHeight - DEFAULT_MAX_HEIGHT_OFFSET}];
}, [isSmallScreenWidth, shouldEnableMaxHeight, windowHeight, styles.createMenuContainer, anchorAlignment, anchorPosition]);☝️ This refactoring is mainly code-wise, it won't fix this issue, you'll have to figure out that one by yourself.
There was a problem hiding this comment.
@ikevin127 thanks for the suggestions 🙇🏻. In the code snippet you provided, I think we don’t need to include [styles.createMenuContainer] on small-width devices when shouldEnableMaxHeight is false, and the same applies to the second condition if (isSmallScreenWidth) {. I’ve removed the style in those cases and updated the code.
Reviewer Checklist
Screenshots/VideosMacOS: Chrome web.movDesktop desktop.mov |
This comment was marked as outdated.
This comment was marked as outdated.
|
🔄 Awaiting code review comments to be addressed before retesting and approving. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Fixed. |
This comment was marked as outdated.
This comment was marked as outdated.
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
Seems good to me - @Expensify/design do you mind giving these videos a final look?
|
😬 Personally I think we should make the menus a little bit shorter so that they feel more like popover menus and so that the last item before the scroll gets clipped in the middle of the item item. @shawnborton what do you think? I remember you making a comment earlier about them being too short—but these really tall ones aren't really feeling like a menu to me. They look almost like an RHP, which is visually confusing me a little bit. |
|
Hmm I think I agree, this feels not so great: So maybe we need to shorten by an extra 40px or something like that and see how it goes? |
|
Also, another consideration here: The top and bottom padding that the menu has should be included in the scrollable area. So I would expect the scrollbar to be as tall as the menu itself, and then the top 16px of padding and bottom 16px of padding scrolls within the overall menu wrapper. |
Agree with you. I also think it's way too tall and would prefer to have a max-height with clipping to indicate scroll. I think seeing that many options at one time is also quite overwhelming. |
|
Yeah big agree with you there. Can we try a max-height around 
To me that feels like plenty of options to display at once. cc @Expensify/design for thoughts! |
|
I think that makes sense, maybe we need to set the height to be something like |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Expensify/design, here is the updated result after adding padding inside the scrollable area and a maximum height of 496px. Monosnap.screencast.2025-10-01.11-28-01.mov |
|
That feels a lot better to me 👍 |
|
Nice, I think that feels pretty great too 👍 |
|
Yeah I agree, that feels much better to me 👍 |
|
Thanks for the update @Krishna2323! Looks like we're failing some checks now, mind taking a look? |
|
For the PR author checklist, 3 new checkboxes were added (49 -> 52) in the meantime: - [ ] If new assets were added or existing ones were modified, I verified that:
- [ ] The assets are optimized and compressed (for SVG files, run `npm run compress-svg`)
- [ ] The assets load correctly across all supported platforms.as for the react compiler workflow, here's a detailed explanation regarding the recent implementation and why we get the errors (it's not just us on this PR). TLDR: The React Compiler (formerly “React Forget”) tries to auto-memoize so you don’t have to write manual useMemo/useCallback. But if you already have memoization and it’s “non-canonical” (e.g. wrapping unstable objects, depending on mutable values, or unnecessary), the check fails. @dangrous Latest: They decided to disable the workflow today. Follow ongoing discussion for ways of moving forward with react compiler. I think we can skip the failing react compiler workflow for this PR, the only thing left here is for the author to add the missing checkboxes to the checklist ✅ cc @Krishna2323 |
|
Thank you for the explanation! Based on that discussion, it does look like we should still update the (new) memoized component in PopoverMenu, since that's fresh code so we should keep it compliant. The other one, though, we can ignore. Do you mind making that change @Krishna2323? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@ikevin127 @dangrous removed the manual memoization from |
|
✋ 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/dangrous in version: 9.2.27-0 🚀
|
|
@Krishna2323 PR failed with the original issue on Native apps |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀
|
8 participants
Explanation of Change
Fixed Issues
$ #70364
PROPOSAL: #70364 (comment)
Tests
Precondition:
Offline tests
QA Steps
Precondition:
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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4