-
Notifications
You must be signed in to change notification settings - Fork 3k
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 #47472
Conversation
@mollfpr In android and ios native, the Lines 42 to 43 in 57ef25c
The To address this, should we create a new Onyx key, |
const {adaptedState} = getAdaptedStateFromPath(ROUTES.ONBOARDING_ROOT.route, linkingConfig.config); | ||
navigationRef.resetRoot(adaptedState); | ||
Navigation.resetToHome(); | ||
Navigation.navigate(Welcome.getInitialOnboardingPath()); |
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 changing this line to Navigation.resetHome
because it causes some bugs when I use the same code in welcome of Report.openReportFromDeepLink
:
Back to personal details
back_to_personal_detaisl-d.mp4
Click next on purpose back to purpose:
click_next_back_to_purpose-d.mp4
src/libs/actions/Report.ts
Outdated
Welcome.isOnboardingFlowCompleted({onNotCompleted: () => Navigation.navigate(ROUTES.ONBOARDING_ROOT.getRoute())}); | ||
Welcome.isOnboardingFlowCompleted({ | ||
onNotCompleted: () => { | ||
Navigation.resetToHome(); |
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.
This will fix the issue where a random URL leads to a "not-found" screen and the onboarding modal will disappears after clicking "next" on the purpose page, as mentioned in the issue.
@mollfpr, there is also a bug in the main: when a user fills out the work page/business name, clicks "continue" to go to the personal details page, then refreshes the page and clicks the back button, the purpose page is displayed instead of the work page. So I think this is out of scope. back to purpose instead of work page:back_to_purpose.mp4 |
Sorry for the delay 🙏
Yes! I think we can implement the original proposal. |
@mollfpr I have added the onboarding last visited path on onyx. |
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.
Sorry for the delay! Mostly the changes and the result look good, but I have an issue after signing up the onboarding modal is not showing until I refresh the page.
Screen.Recording.2024-08-22.at.21.10.52.mp4
} | ||
|
||
onboardingInitialPath = value.substring(1); | ||
Onyx.disconnect(onboardingLastVisitedPathConnection); |
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.
Why do we need to disconnect 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.
Because we only need the initial value when the user opens the app or URL. I have tested it, the onboarding is triggered from bottomTabBar
and openReportFromDeepLink
, leading to navigation changes that affect the lastVisitedPath
and will open incorrect page.
I tested mostly on legacy mode. |
This comment was marked as outdated.
This comment was marked as outdated.
Thanks for the investigation @tsa321! Sure, I'll give it a try! Just wondering if this will be the production build or not. |
dded728
to
7a13a23
Compare
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
Navigation.dismissModal(); | ||
// Navigate to HOME instead of dismissModal, because there is bug in small screen | ||
// where the onboarding puropose page will be disaplayed briefly | ||
Navigation.navigate(ROUTES.HOME); |
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 changing dismissModal
to navigate HOME
because there is a bug:
Onboarding purpose disaplyed briefly:
home-instead-of-dismiss-d.mp4
@@ -180,6 +202,11 @@ function completeHybridAppOnboarding() { | |||
API.write(WRITE_COMMANDS.COMPLETE_HYBRID_APP_ONBOARDING, {}, {optimisticData, failureData}); | |||
} | |||
|
|||
function startOnboardingFlow() { | |||
const {adaptedState} = getAdaptedStateFromPath(getOnboardingInitialPath(), linkingConfig.config, 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.
I really need to add an optional parameter to getAdaptedStateFromPath
because it is causing this bug:
The path is being appended to the URL inserted by the user:
Incorrect URL path:
incorrect-path-d.mp4
onCompleted?.(); | ||
} else { | ||
} else if (!isOnboardingInProgress) { |
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.
Prevents multiple onboarding flow running causing navigation issue.
@@ -365,7 +365,7 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList> | |||
}; | |||
} | |||
|
|||
const getAdaptedStateFromPath: GetAdaptedStateFromPath = (path, options) => { | |||
const getAdaptedStateFromPath: GetAdaptedStateFromPath = (path, options, shouldReplacePathInNestedState = 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 am adding an optional parameter here, I really need it, more info here.
Reviewer Checklist
Screenshots/VideosAndroid: Native47472.Android.mp4Android: mWeb Chrome47472.mWeb-Chrome.mp4iOS: Native47472.iOS.moviOS: mWeb Safari47472.Safari.movMacOS: Chrome / Safari47472.Web.mp4MacOS: Desktop47472.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 👍
Also, the result on web still good under the strict mode true
.
I had to revert this PR since it caused a number of workflow failures. Please create another PR and ensure you are up to date so that GH actions runs on the latest code 🙂 |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.0.26-1 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.26-6 🚀
|
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