-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Better RHP view v4 - Use wide RHP for receipts #69875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better RHP view v4 - Use wide RHP for receipts #69875
Conversation
c6ee6ea to
2fe6b97
Compare
|
@parasharrajat 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] |
trjExpensify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell yeah, can't wait for this! 👍
|
@adamgrzybowski please add detailed test steps to this PR. |
|
@parasharrajat description updated |
|
Waiting on 3rd PR to be merged. |
71570ce to
09146ee
Compare
|
@adamgrzybowski Let me know when this is ready. |
|
@parasharrajat it is ready for the review 🫡 |
src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx
Outdated
Show resolved
Hide resolved
|
Please use merge commits instead of rebasing as project best practices. |
|
Looks like we have got 2 legitimate bugs. @WojtekBoman |
|
I guess we need to revert the PR if we can't solve these cases immediately @WojtekBoman |
Let's wait to catch all regressions related to this feature before we revert, if possible. I'm looking at these bugs 🕵️♂️ |
|
If you can solve those then we don't need to revert. Most of these are quick ones. |
|
@WojtekBoman is working on the fixed but created the revert draft so we an also more easily confirm if some issues are coming from this PR #71980 discussing stuff here https://expensify.slack.com/archives/C01GTK53T8Q/p1759833496866449 |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.26-7 🚀
|
| }); | ||
|
|
||
| const wideRHPRouteKeys = useMemo(() => { | ||
| const rootState = navigationRef.getRootState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a "'navigation' object hasn't been initialized yet" console error on https://staging.new.expensify.com/.
| isFromReviewDuplicates={isFromReviewDuplicates} | ||
| mergeTransactionID={mergeTransactionID} | ||
| report={report} | ||
| onLoad={() => setIsLoading(false)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't set loading to false on failure of load case causing this issue - #72444
| // When a report with one expense is opened in the wide RHP and the user adds another expense, RHP should be dismissed and ROUTES.SEARCH_MONEY_REQUEST_REPORT should be displayed. | ||
| if (hasMultipleTransactions && reportID) { | ||
| Navigation.dismissModal(); | ||
| InteractionManager.runAfterInteractions(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using InteractionManager.runAfterInteractions caused #76657 because the modal wasn’t fully dismissed on the native app. We should use setNavigationActionToMicrotaskQueue instead.
Explanation of Change
This PR uses the functionality of wide RHP for the expenses to show the receipt view side by side with the rest of the expense
It's a part of the bigger chain of changes:
Fixed Issues
$ #64025
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Without a receipt
With a distance receipt
With an e-receipt receipt
With a pdf receipt
With a photo receipt
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: mWeb Chrome
desktop.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
iosWeb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4