-
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
Refactor NewDot and HybridApp onboarding flow #49586
Refactor NewDot and HybridApp onboarding flow #49586
Conversation
src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
Outdated
Show resolved
Hide resolved
This branch has conflicts @blazejkustra can you take a look |
…labs/expensify-app-fork into fix/onboarding-offline
Umm, fair point, I will see testrails just in case we have better coverage there. My email would be: [email protected] |
Thread for better discussion here |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Onboarding - App opens /r/undefined when opening onboarding steps after onboarding is doneVersion Number: 9.0.40-4 Action Performed:
Expected Result:App will return to any report because the onboarding is complete Actual Result:App redirects to https://49586.pr-testing.expensify.com/r/undefined Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6617639_1727458929014.20240928_013704.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Sign in – Blank page opens when log in as a user with 2FAVersion Number: 9.0.40-4 Action Performed:
Expected Result:Inbox opens Actual Result:Blank page opens Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6617650_1727459574481.And.1.mp4 |
This is connected to a bigger regression that was introduced in AuthScreens withOnyx migration. After reverting locally it works fine: Screen.Recording.2024-09-30.at.15.07.23.mov |
Actually it was just reverted, so I'm going to focus on the other error now 👀 |
As for the other bug: Sign in – Blank page opens when log in as a user with 2FA It's reproducible on the newest main (only on bad internet speed), so I wouldn't say it's connected to this refactor. Screen.Recording.2024-09-30.at.15.39.59.mov |
The issue isn't as visible on the main because of the above revert, but it hasn't been fully resolved either. Regardless, this PR isn't the source of these bugs, so you can continue reviewing, @allgandalf. 🚀 |
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 code looks great, thanks for all the hard work and the refactor, code is so much clearer now, I only have a few comments, I will start with the checklist in the meantime
// Backend might return strings instead of booleans | ||
if (typeof completedHybridAppOnboarding === 'string') { |
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, but why exactly this ? There might be a fixed type right on the BE
? This is a safe typecheck but still thinking 🤔
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 copied over from src/libs/actions/Welcome/index.ts
, BE sends completedHybridAppOnboarding
as a 'true' | 'false'
sometimes, and we need to handle them accordingly 😄
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2024-09-30.at.8.14.54.PM.movScreen.Recording.2024-09-30.at.8.22.14.PM.movScreen.Recording.2024-09-30.at.8.20.14.PM.movMacOS: DesktopScreen.Recording.2024-09-30.at.8.36.09.PM.movAndroid: NativeScreen.Recording.2024-09-30.at.8.43.27.PM.movAndroid: mWeb ChromeScreen.Recording.2024-09-30.at.8.45.37.PM.moviOS: NativeScreen.Recording.2024-09-30.at.8.37.47.PM.moviOS: mWeb SafariScreen.Recording.2024-09-30.at.8.28.40.PM.mov |
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.
Great work @blazejkustra 🙇 , This looks great to me, lets get the refactor through 🚀
All yours @mountiny @carlosmiceli 🙌 |
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.
Changes look good to me, thanks!
We are going to skip the |
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not emergency, explained above |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.42-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.42-3 🚀
|
Details
HybridApp PR: Adjust OldDot to new fix of NewDot onboarding
While working on #49499, I noticed how complicated the current onboarding logic is:
Current onboarding issues
First of all there are two onboarding flows:
HybridApp
onboarding that displays Explanation Modal - it is triggered from the top fileExpensify.tsx
with some conditional logic that is unnecessary complicatedNewDot
onboarding is triggered conditionally, this time fromExplanationModal.tsx
NewDot
onboarding that displays Onboarding Modal - it is triggered fromBottomTabNavigator
but only if HybridApp module isn't available (also the logic is quite complex)There is no single source of truth on how onboarding should behave, instead it is scattered across the codebase making this refactor quite hard, these files affect onboarding:
Expensify.tsx
ExplanationModal.tsx
OnboardingModalNavigator.tsx
BottomTabBar.tsx
Currently we are relying on the backend to change the value for
NVP_ONBOARDING
, which obviously works in most cases, but when being offline this causesNVP_ONBOARDING
to never change even after onboarding completion.OnboardingModalNavigator.tsx
that returnsnull
when NewDot onboarding is completeddismissModal
NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR
is always mounted while the screens inside are rendered conditionally - which is anti-pattern - that's why after consulting with @adamgrzybowski we decided to hide or display the whole navigator fromAuthScreens
This PR aims to bring all the logic into one place
useOnboardingFlow
which controls the whole onboarding flow, making it much more simple.Fixed Issues
$ #49499
PROPOSAL: N/A
Tests
Hybrid App Tests (No QA)
Offline 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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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
iOS: Native
Simulator.Screen.Recording.-.iPhone.taki.SE.-.2024-09-30.at.16.28.40.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-09-30.at.16.29.13.mov