-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add backTo to many pages so it shows the correct screen below the RHP overlay #47990
base: main
Are you sure you want to change the base?
Add backTo to many pages so it shows the correct screen below the RHP overlay #47990
Conversation
@rayane-djouah 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] |
Additional video for other pages: Report settings: web.settings.mp4Money request view data: web.money.request.data.mp4Profile from message: web.profile.mp4Report field: web.report.field.mp4New task page: web.task.mp4 |
For this, I didn't add the
which opens the hold educational again. |
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.
Left a small comment
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid_native.mp4Android: mWeb Chromeandroid_mweb_chrome.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-20.at.01.21.33.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-20.at.01.19.52.mp4MacOS: Chrome / SafariScreen.Recording.2024-09-20.at.1.15.45.AM.movScreen.Recording.2024-09-21.at.11.14.29.PM.movScreen.Recording.2024-09-21.at.11.23.49.PM.movMacOS: DesktopScreen.Recording.2024-09-20.at.1.17.42.AM.mov |
After merging with main, the page without params will now return undefined if we access |
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.
LGTM
@bernhardoj - We still need to add
Note that in #47690 we will be adding the chat type to search, therefore more report-related routes will be accessible from RHP, therefore I think we need to add the
wdyt? |
Please merge latest main. Thanks! |
I would prefer to wait for it to be merged so I can test it. In the meantime, I'll add more backTo to these pages
|
web.mp4 |
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.
Let's add backTo
param to FLAG_COMMENT
route
@bernhardoj we also have conflicts |
@bernhardoj we have conflicts |
@rayane-djouah conflicts are resolved. Let's resume review. |
|
||
type PromotedActionsType = Record<BasePromotedActions, (report: OnyxReport) => PromotedAction> & { | ||
share: (report: OnyxReport, backTo?: string) => PromotedAction; |
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.
share: (report: OnyxReport, backTo?: string) => PromotedAction; | |
CONST.PROMOTED_ACTIONS.SHARE: (report: OnyxReport, backTo?: string) => PromotedAction; |
// No change needed, navigate back | ||
if (previousName !== policyRoomName) { | ||
ReportActions.updatePolicyRoomName(report, policyRoomName); | ||
return; |
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 still need to go back if no change is needed
return; |
const policyRoomName = values.roomName; | ||
// No change needed, navigate back | ||
if (previousName !== policyRoomName) { | ||
ReportActions.updatePolicyRoomName(report, policyRoomName); |
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 remove this code from ReportActions.updatePolicyRoomName
function here :
// No change needed, navigate back
if (previousName === policyRoomName) {
Navigation.goBack(ROUTES.REPORT_SETTINGS.getRoute(reportID));
return;
}
Let's update it to:
// No change needed
if (previousName === policyRoomName) {
return;
}
and remove the previousName !== policyRoomName
check in the updatePolicyRoomName
reference here
src/libs/actions/Report.ts
Outdated
if (previousValue === newValue) { | ||
if (navigate && !isEmptyObject(report) && report.reportID) { | ||
ReportUtils.goBackToDetailsPage(report); | ||
} | ||
return; | ||
} |
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.
Let's keep the return check here:
if (previousValue === newValue) {
return;
}
and remove the previousValue === newValue
condition in updateNotificationPreference
references
src/libs/actions/Report.ts
Outdated
// No change needed, navigate back | ||
if (previousValue === newValue) { | ||
Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(reportID)); | ||
return; | ||
} |
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.
Let's keep the return check here:
if (previousValue === newValue) {
return;
}
and remove the previousValue === newValue
condition in updateDescription
references
src/libs/actions/Report.ts
Outdated
if (report.writeCapability === newValue) { | ||
Navigation.goBack(ROUTES.REPORT_SETTINGS.getRoute(report.reportID)); | ||
return; | ||
} |
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.
Let's keep the return check here:
if (report.writeCapability === newValue) {
return;
}
and remove the report.writeCapability === newValue
condition in updateWriteCapability
references
@rayane-djouah done |
@bernhardoj we have conflicts again |
The code looks good to me, and I’m actively testing the changes. |
Conflict solved |
Thanks! Let's get this to the finish line @rayane-djouah |
@bernhardoj Could we address the following layout bug? I think we need to use App/src/components/MoneyReportHeader.tsx Lines 428 to 435 in 25450d9
App/src/components/MoneyRequestHeader.tsx Lines 188 to 194 in 25450d9
|
Bug: the back button loops between the profile page & notification preferences page after refresh: VideoScreen.Recording.2024-09-20.at.1.52.31.AM.mov |
Fixed both. |
src/components/MoneyReportHeader.tsx
Outdated
setShouldShowHoldMenu(isOnHold && !dismissedHoldUseExplanation); | ||
setShouldShowHoldMenu(true); |
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 believe this was changed for testing only, right?
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.
Oops, you're right. Thanks for catching that! Reverted.
setShouldShowHoldMenu(isOnHold && !dismissedHoldUseExplanation); | ||
setShouldShowHoldMenu(true); |
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.
Same here
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.
LGTM and tests well 👍
@luacmartins all yours! |
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.
Oops! Just found a bug:
Screen.Recording.2024-09-21.at.11.45.20.PM.mov
@bernhardoj I think it would be better to make the |
I think we can keep it optional, otherwise I need to re-check all ROUTES to see which backTo can be optional or not. backTo by pattern is also optional.
Fixed. |
Details
While we open a page from report RHP, refreshing will show the report screen below the report RHP instead of the search page. This PR fixes it.
Fixed Issues
$ #46773
PROPOSAL: #46773 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: the account never dismisses hold educational page/modal yet
Web/Desktop
Android/iOS/mWeb
Can't see page behind the RHP, so just make sure the hold educational modal still shows normally
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
androi.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4