[Landscape] Fix popup content is not fully shown#88191
[Landscape] Fix popup content is not fully shown#88191bernhardoj wants to merge 9 commits intoExpensify:mainfrom
Conversation
|
@aimane-chnaif 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa7f24bbb
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I'll explain the solution, but before that, we need to understand the root cause first.
The first problem here is that we don't take into account the sort order menu item height, so when used in landscape mode, there isn't enough space for the sort order menu. App/src/components/Search/FilterDropdowns/SortByPopup.tsx Lines 57 to 78 in bfba460 But even if we take into account the sort order menu item height, the issue still won't be fully fixed. We can notice this height issue in other popups that use the same Here is how it works: Line 6305 in bfba460 ESTIMATED_LIST_HEIGHT: MAX_HEIGHT:
On the web, with mobile view, if we reduce the height to be really small, then the list will take the 90% of the height, and the footer will be hidden because of not enough space. MAX_HEIGHT calculation doesn't take into account when the popup has a header with a back button instead of a label/title. So, when there is a mismatch, the list will take too much space, and the header or buttons could be cut. This is why I decided to use |
| return {height}; | ||
| }, | ||
|
|
||
| getPopoverMaxHeight: (windowHeight: number, isInLandscapeMode: boolean) => { |
There was a problem hiding this comment.
Created this for a non-selection list popup.
| restoreFocusType={CONST.MODAL.RESTORE_FOCUS_TYPE.DELETE} | ||
| shouldEnableNewFocusManagement | ||
| shouldMeasureAnchorPositionFromTop={false} | ||
| shouldUseModalPaddingStyle={false} |
There was a problem hiding this comment.
On a small screen, a bottom-docked modal will be used, and by default, it have a 16px vertical padding applied. That's why we had !isSmallScreenWidth && styles.pv4 everywhere. I disabled this, so we can apply the padding to all screens, and we don't need to conditionally account for the modal vertical padding anymore in getSelectionListPopoverHeight.
|
@JS00001 can you please generate adhoc build? |
|
🚧 @JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@aimane-chnaif fixed |
|
Please generate new build again |
|
Running |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Code Review — PR #88191 The approach of unifying Bug: In style={[styles.getSelectionListPopoverHeight({itemCount: listData.length || 1, windowHeight, isInLandscapeMode, hasTitle})]}The old code passed style={[styles.getSelectionListPopoverHeight({itemCount: listData.length || 1, windowHeight, isInLandscapeMode, hasTitle, isSearchable})]}Everything else looks good — all callers migrated to the new API, |
| <View style={[styles.pv4]}> | ||
| <View style={styles.getSelectionListPopoverHeight({itemCount: groupByOptions.length || 1, windowHeight, isInLandscapeMode, hasButton: false})}> |
|
This should be scrollable too in case various filters are applied Screen.Recording.2026-04-20.at.8.27.14.pm.mov |
Can you get to it at all if you scroll the page? |
The scroll behavior is not consistent. Screen.Recording.2026-04-20.at.8.45.36.pm.mov |
|
My gut reaction is to just not fix all those headers and controls in landscape mode, but I'm curious to hear what @Expensify/design thinks. |
|
Depends on the priority here. I think we would ideally scroll the search bar and header in landscape and then on scroll down you'd reveal them. I could actually see that working well for regular screens too. If it's not a big priority, I'm fine with leaving it though |
|
Agree with leaving it. @bernhardoj Let's just fix #88191 (comment) & #88191 (comment) |
|
I think I'll use a different approach because of this. I'll explain it more later. |
|
Currently, our popup looks like this In #88428, I extracted the content to make it reusable for the new filters popup. Since the content data will live in the extracted content component, the parent won't have the information of the data length anymore. So, I decided to move the height style to the content component. |





Explanation of Change
Fixed Issues
$ #88051
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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