-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make onboarding continue from last visited onboarding page #48137
Conversation
Signed-off-by: Tsaqif <[email protected]>
@mollfpr the previous PR was reverted due to lint and type-check errors caused by the deletion of ROUTES in |
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.
The changes still look good. However, after retesting it I think these found will be taken as a regression.
- The
Onboarding_Purpose
is shown briefly after refreshing the page. - The back button from
Onboarding_Personal_Details
doesn't take to theOnboarding_Work
after refreshing the page.
Screen.Recording.2024-08-29.at.12.24.58.mp4
I agree the above issue might be out of the scope of this PR, unfortunately, it's introduced in this PR.
So what do you guys think, should we fix it in this PR?
@@ -73,7 +74,9 @@ function BaseOnboardingPersonalDetails({ | |||
Welcome.setOnboardingAdminsChatReportID(); | |||
Welcome.setOnboardingPolicyID(); | |||
|
|||
Navigation.dismissModal(); | |||
// Navigate to HOME instead of dismissModal, because there is bug in small screen | |||
// where the onboarding puropose page will be disaplayed briefly |
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.
// where the onboarding puropose page will be disaplayed briefly | |
// where the onboarding purpose page will be displayed briefly |
@mollfpr, I can actually reproduce the first issue in staging as well. However, due to the optimizations in the production build, the purpose page is displayed for a shorter time compared to development. bug-d.mp4 |
This also happens in main/staging. Here is the video. To fix this will require changes in the Navigation/Routes-related files, I am afraid it would break other navigation function
This also happens in the So, I believe these two bugs already exist in the main branch. |
If the bug exists in main branch then we don't need to solve it here. |
@chiragsalian I would like to try to fix the issues here. I will inform you again once I am able to fix the issues. |
…oing back navigation Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
@@ -111,7 +112,6 @@ function BaseOnboardingWork({shouldUseNativeStyles, onboardingPurposeSelected, o | |||
shouldSaveDraft | |||
maxLength={CONST.TITLE_CHARACTER_LIMIT} | |||
spellCheck={false} | |||
autoFocus |
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 am removing this line because the autofocus is handled by useAutoFocusInput();
as it is in the personal details page. This line is causing issues with the work page animation (the animation doesn't run) and navigation problems on iOS.
I have fixed the two issues mentioned above. |
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 👍
Just need to fix the minor suggestion and we're good to go.
While testing back and forth, I found an issue where the page redirects to the personal details page when pressing the browser back button on the onboarding work page. After that, I test the same scenario on staging and it's also reproducible, so the issue is not coming from this PR.
Screen.Recording.2024-09-04.at.21.08.46.mp4
{ | ||
name: SCREENS.ONBOARDING.WORK, | ||
params: currentRoute?.params, | ||
path: '/onboarding/works', |
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.
The path is work
singular. I think it's better we use getRoute()
.
path: '/onboarding/works', | |
path: `/${ROUTES.ONBOARDING_WORK.route}`, |
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.
@mollfpr actually we don’t need the path field, I forgot to remove it.
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
Reviewer Checklist
Screenshots/VideosAndroid: Native48137.Android.mp4Android: mWeb Chrome48137.mWeb-Chrome.mp4iOS: Native48137.iOS.moviOS: mWeb Safari48137.mWeb-Safari.movMacOS: Chrome / Safari48137.Web.mp4MacOS: Desktop48137.Desktop.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.
LGTM 👍
- The Onboarding_Purpose is shown briefly after refreshing the page.
- The back button from Onboarding_Personal_Details doesn't take to the Onboarding_Work after refreshing the page.
I confirm the above issue is fixed!
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
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
This PR is failing for Desktop because of issue #48937 |
function goBack() { | ||
adaptOnboardingRouteState(); | ||
Navigation.isNavigationReady().then(() => { | ||
Navigation.goBack(); |
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.
Coming from the BZ of #50177
This condition is missing handling for the case where the user navigates back using the hardware button on Android and the onboarding route state does not initialize properly. The solution for this issue is to reset the onboarding state when setting up the router.
Details
Fixed Issues
$ #46104
PROPOSAL: #46104 (comment)
Tests
Join
Manage my team's expenses
..com
, for example,/aaa
(for desktop, enter the URL in the browser asnew-expensify:/aaa
and press Enter)..com
to/settings/profile
(for desktop, enter the URL in the browser asnew-expensify:/settings/profile
and press Enter).Continue
.Offline tests
QA Steps
Join
Manage my team's expenses
..com
, for example,/aaa
(for desktop, enter the URL in the browser asnew-expensify:/aaa
and press Enter)..com
to/settings/profile
(for desktop, enter the URL in the browser asnew-expensify:/settings/profile
and press Enter).Continue
.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
android-native-d.mp4
Android: mWeb Chrome
android-mweb_d.mp4
iOS: Native
ios-native_d.mp4
iOS: mWeb Safari
ios-msfari_d.mp4
MacOS: Chrome / Safari
macos-web-d.mp4
MacOS: Desktop
macos-desktop_d.mp4