-
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
Tasks for guided setup #39951
Tasks for guided setup #39951
Conversation
…tructure for onboarding
…m/barttom/App-expensify into feature/38771-tasks-for-guided-setup
…tructure for onboarding
…m/barttom/App-expensify into feature/38771-tasks-for-guided-setup
…ture/38771-tasks-for-guided-setup
…ture/38771-tasks-for-guided-setup
…ture/38771-tasks-for-guided-setup
Update: PR is LGTM, and tests well. We are waiting now for a response regarding #39951 (comment) and #39951 (comment). Also, There are known backend bugs #39951 (comment) and #39951 (comment), but these should be NAB for this PR. |
Also, cc'ing @Expensify/design to review the PR |
Yep noting these are known We are moving to using a new NVP: I think that the changes here are minor and we could merge this first and then retest the enablement of the flow. @rayane-djouah could you complete the checklist? |
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
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.
Non blocking comments, I would love to get this merged to main before we enable the onboarding flow
@@ -369,7 +369,7 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie | |||
/> | |||
<RootStack.Screen | |||
name={NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR} | |||
options={onboardingScreenOptions} | |||
options={screenOptions.fullScreen} |
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 is I assume only for testing now, 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.
I'm not sure about this one, onboardingScreenOptions
come from here
const onboardingScreenOptions = useMemo(() => screenOptions.onboardingModalNavigator(shouldUseNarrowLayout), [screenOptions, shouldUseNarrowLayout]);
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 can handle that in the follow up PR. thanks
src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx
Outdated
Show resolved
Hide resolved
src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
Outdated
Show resolved
Hide resolved
src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
Outdated
Show resolved
Hide resolved
Thank you very much! I have tested this out and we are going to move the finishing touches to the Enablement PR. I think we need to remove the videos for phase 1 Screen.Recording.2024-04-21.at.22.24.12.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.
Thanks @fabioh8010 for help ❤️
this PR should not influence the user experience on main. The flow is going to be enabled in this PR #39687 and we are going to work on Monday to clean everything up
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@rezkiy37 Hello |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.64-6 🚀
|
// Only navigate to concierge chat when central pane is visible | ||
// Otherwise stay on the chats screen. | ||
if (isSmallScreenWidth) { | ||
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.
@rezkiy37 , why did we navigate to HOME
instead of Concierge on small devices ? We have a GH issue here which has expected result that we should always navigate to concierge on all devices, was this intentional ? c.c. @mountiny and @rayane-djouah as they seem to have reviewed the PR
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.
Yes, it was intentional. The doc says:
In the component OnboardingPurpose we can find a method that sends a message as the Concierge persona. After this action, the user should be able to see the video modal and be navigated to the root screen.
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.
Cool thanks a lot 🙏
if (networkTimeSkew > 0) { | ||
return getDBTime(new Date().valueOf() + networkTimeSkew); | ||
return getDBTime(new Date(timestamp).valueOf() + networkTimeSkew); |
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.
Looks like this caused an issue #40724 preventing chat to scroll on new messages. When timestamp is not passed new Date('').valueOf()
becomes NaN
.
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.
Thanks!
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${targetChatReportID}`, | ||
value: { | ||
lastMentionedTime: DateUtils.getDBTime(), |
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 haven't provided lastVisibleActionCreated
optimistically for the target chat Report here, so the Concierge chat can auto scroll to the bottom after onboarding process. #52025
tasks: [ | ||
{ |
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 did not include selfGuidedTourTask
in the tasks if the tour purpose is combinedTrackSubmitOnboardingChoices.PERSONAL_SPEND
leading to this issue.
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.
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 that and thanks a lot for tracking. I will post i there
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.
No problem! :)
Details
The PR introduces new onboarding flow. The app uses the new API command CompleteGuidedSetup and pass new tasks and message's optimistic data, on behalf of Concierge. After, the app redirects user to root screen.
Fixed Issues
$ #38771
PROPOSAL: N/A
Tests
Note: To test with an existing user, you can initiate the flow from Settings > Troubleshoot > Onboarding Flow.
With the creation of a workspace (Track business expenses..., Manage my team’s...)
Without the creation of a workspace (others)
Offline tests
Same as Tests.
QA Steps
Same as Tests.
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.mp4
Android: mWeb Chrome
Android.Chrome.mp4
iOS: Native
iOS: mWeb Safari
IOS.Safari.mp4
MacOS: Chrome / Safari
Chrome.Track.mp4
Chrome.Chat.mp4
MacOS: Desktop
Desktop.mp4