Align device and in-app back navigation in "Paid" filter section#83154
Align device and in-app back navigation in "Paid" filter section#83154arosiclair merged 25 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
1. Date date.mov2. Approved approved.mov3. Submitted submitted.mov4. Exported exported.mov5. Withdrawn withdrawn.mov |
…er-back-nav-alignment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
@marufsharifi TS is still failing, can you take a look? 🙏 |
|
@jjcoffee, done, please take another look. thanks. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-app-2026-03-05_17.24.53.mp4Android: mWeb Chromeandroid-chrome-2026-03-05_17.29.04.mp4iOS: HybridAppios-app-2026-03-05_17.14.55.mp4iOS: mWeb Safariios-safari-2026-03-05_17.16.27.mp4MacOS: Chrome / Safaridesktop-chrome-2026-03-05_17.10.23.mp4 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@marufsharifi Can you also write correct test steps? They seem to be copied from the issue. |
|
@marufsharifi Looks like the design has changed a bit by now. The on/before/after modes are now selectors and so I guess shouldn't lead to subpages. With this PR we have weird navigation animation when switching between the modes: desktop-chrome-retest-2026-03-12_14.33.33.mp4 |
|
@jjcoffee I think the design was reverted. Could you please take another look? Thanks! Screen.Recording.1404-12-23.at.2.49.58.PM.mov |
|
@jjcoffee, gentle bump. thanks. |
|
@marufsharifi Oh nice, that saves some work! One last nitpick here, I think it makes more sense if the sub page paths are all lowercase - it's a bit awkward to have capitalised path in a URI I think. Unless we've already used this pattern elsewhere? |
@jjcoffee, I've addressed your comment. Please take another look. thanks. |
| buildRoute: buildSubPageRoute, | ||
| }); | ||
|
|
||
| const selectedDateModifier = useMemo<SearchDateModifier | null>(() => { |
There was a problem hiding this comment.
It doesn't look like this needs to be memoized since it's just a few if statements. Can we just compute this value plainly?
| const DATE_FILTER_SUB_PAGES: Array<PageConfig<SubPageProps>> = [ | ||
| {pageName: CONST.SEARCH.DATE_FILTER_SUB_PAGE.MAIN, component: EmptySubPageComponent}, | ||
| {pageName: CONST.SEARCH.DATE_FILTER_SUB_PAGE.ON, component: EmptySubPageComponent}, | ||
| {pageName: CONST.SEARCH.DATE_FILTER_SUB_PAGE.AFTER, component: EmptySubPageComponent}, | ||
| {pageName: CONST.SEARCH.DATE_FILTER_SUB_PAGE.BEFORE, component: EmptySubPageComponent}, | ||
| ]; |
There was a problem hiding this comment.
Hmm It's kind of odd that we're using useSubPage like this. I didn't realize this is what we would end up doing in the proposal either. It seems like we're not really using most of the hook's functionality, so can we just use regular page navigation instead to implement this? @marufsharifi
|
@arosiclair, I've addressed your comments, please take another look. thanks. |
| Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(redirectTo, {forceReplace: true})); | ||
| }, [domainAccountID, domain?.validated, doesDomainExist, redirectTo]); | ||
| Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(redirectToPath as Route, {forceReplace: true})); | ||
| }, [domain?.validated, doesDomainExist, redirectToPath]); |
There was a problem hiding this comment.
Why is this change needed?
There was a problem hiding this comment.
This change was added by mistake while I was fixing a type issue. I've reverted it. Thanks
src/ROUTES.ts
Outdated
| if (!subPage || !filterKey) { | ||
| return baseRoute; | ||
| } | ||
| return `${baseRoute}/${encodeURIComponent(subPage)}` as const; |
There was a problem hiding this comment.
| return `${baseRoute}/${encodeURIComponent(subPage)}` as const; | |
| return `${baseRoute}/${subPage}` as const; |
encodeURIComponent seems unnecessary here since the subPage should just be plain text
|
@arosiclair, I've addressed your comments. Could you please take another look? Thanks! |
|
@jjcoffee we changed the implementation a little bit. Can you retest? |
|
@arosiclair Still working fine for me on web and native! |
|
🚧 @arosiclair 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/arosiclair in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
Explanation of Change
Previously, selecting a date mode (
On,After,Before) lived only in the local component state, so hardware/browser back could conflict with what users saw on screen. Now, the date filter step is represented in the URL as a subpage (main,On,After,Before) and managed throughuseSubPage.Fixed Issues
$ #70180
PROPOSAL: #70180 (comment)
Tests
advance filter > paid > before.Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-02-24.at.10.22.53.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-02-24.at.10.25.40.AM.mov
iOS: Native
Screen.Recording.2026-02-24.at.10.32.34.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-02-24.at.10.39.16.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-02-24.at.10.21.42.AM.mov