Repository navigation
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
I'll review this PR in a couple of hours |
Reviewer Checklist
Screenshots/Videosios.mov |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
🚧 JS00001 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! 🧪🧪
|
|
✋ 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/JS00001 in version: 9.5.28-4 🚀
|
|
No help site update needed. This PR is a layout fix that keeps the |
|
Deploy Blocker #105706 was identified to be related to this PR. |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.30-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
#104668 added a search input to the top of the Search
Filterspage. It sits between the fixed header and the fixedReset/Apply filtersfooter. On a phone in landscape, focusing the input opens the keyboard. That leaves too little height for the header, input and footer together, so the input is pushed out of view and can't be scrolled back.This PR uses
useShouldFooterBeInsideList, the hook other selection pages already use for this case. On a phone in landscape with the keyboard open,ResetandApply filtersmove into the list and scroll after the last filter, which frees the space for the search input. In every other case the buttons stay pinned to the bottom as before.FilterListgets afooterContentprop, rendered after the filter rows inside its scroll view. It mirrors the existingheaderContentprop. The desktop filters popup doesn't pass it, so it is unchanged.SearchAdvancedFiltersBasemoves the buttons into afooterconstant. It passes them toFilterListwhen the hook returns true, and renders them below the list otherwise.Fixed Issues
$ #105228
PROPOSAL: N/A
Tests
Landscape with the keyboard open (Android, iOS, mWeb)
Spend>Expenses.Filters. VerifyApply filtersis pinned at the bottom.Filterspage so the keyboard opens.date. Verify the matching filters are listed.Apply filtersappears after the last filter.Apply filters. Verify the page closes and the filters are applied.Filtersagain, change any filter soResetappears, then focus the search input. Scroll to the end and verifyResetandApply filtersare side by side after the last filter.Portrait (Android, iOS, mWeb)
Filtersand tap the search input.Apply filtersstays pinned above the keyboard as before.Desktop (web, desktop app)
Filtersin a narrow window so it opens as a page. VerifyApply filtersis pinned at the bottom as before.Offline tests
QA Steps
Same as Tests.
Verify that no errors appear in the JS console
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_hybrid.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web_chrome.mp4