-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Test Drive][Phase 1][FE] Show the Test Drive modal during onboarding and via task #60085
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
[Test Drive][Phase 1][FE] Show the Test Drive modal during onboarding and via task #60085
Conversation
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.
@pac-guerreiro Let's prepare the test steps and screenshots/videos in advance while the first PR is in review
…guerreiro/feature/show-test-drive-modal-during-onboarding
…arding' into pac-guerreiro/feature/implement-test-drive-embedded-ui # Conflicts: # src/libs/Navigation/types.ts
…guerreiro/feature/show-test-drive-modal-during-onboarding
…arding' into pac-guerreiro/feature/implement-test-drive-embedded-ui
Today I added testing steps, screen recordings and filled checklist. |
…during-onboarding
…arding' into pac-guerreiro/feature/implement-test-drive-embedded-ui # Conflicts: # src/components/TestDriveModal.tsx
@danieldoglas @rojiphil The PR is ready for review! 😄 |
@pac-guerreiro @danieldoglas @rojiphil I think it will be better to also put the changes of the Embedded UI PR into this one, because this feature is not behind any betas and users would see a modal that doesn't do anything unless the Embedded UI PR is merged. @pac-guerreiro This should be fairly easy to do, and you can also just pull the test steps/screenthots from there and use it here. What do you all think? |
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.
Yeah. I also think merging the embedded UI PR with this one may be better.
@pac-guerreiro I have just left a query. Please have a look. Thanks.
src/libs/navigateAfterOnboarding.ts
Outdated
@@ -13,6 +16,13 @@ const navigateAfterOnboarding = ( | |||
) => { | |||
Navigation.dismissModal(); | |||
|
|||
if (onboardingPurposeSelected === CONST.ONBOARDING_CHOICES.MANAGE_TEAM) { | |||
setTimeout(() => { |
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 is there a need to use setTimeout
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.
We need a delay otherwise the navigation actions (dismissModal and this navigation) will conflict and cause bugs, but @pac-guerreiro we can use InteractionManager.runAfterInteractions()
here instead, it's a better choice
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.
Ah! I get it now. Thanks.
Would this utility setNavigationActionToMicrotaskQueue help 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.
@fabioh8010 @rojiphil InteractionManager.runAfterInteractions()
was not working on native Android and iOS, so I used this logic
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.
@pac-guerreiro Did you check if this 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.
I think we missed addressing this comment. What do you think of this?
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.
@rojiphil I applied your suggestions but still need to test it
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.
@rojiphil I confirm your suggestion works! But I tried using InteractionManager.runAfterInteractions()
again and it also works... I guess I might've stumbled into a cache issue of my emulators 😅 I'll go with @fabioh8010 solution 😄
src/libs/navigateAfterOnboarding.ts
Outdated
@@ -13,6 +16,13 @@ const navigateAfterOnboarding = ( | |||
) => { | |||
Navigation.dismissModal(); | |||
|
|||
if (onboardingPurposeSelected === CONST.ONBOARDING_CHOICES.MANAGE_TEAM) { | |||
setTimeout(() => { |
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 need a delay otherwise the navigation actions (dismissModal and this navigation) will conflict and cause bugs, but @pac-guerreiro we can use InteractionManager.runAfterInteractions()
here instead, it's a better choice
src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx
Outdated
Show resolved
Hide resolved
I'm discussing internally if we should have a beta for this. But I agree that if we don't, then we should merge them together (or merge the embedded PR first) |
I'll wait for your decision @danieldoglas 😉 |
… into pac-guerreiro/feature/show-test-drive-modal-during-onboarding
…during-onboarding
@danieldoglas As decided here, I proceeded with merging the third PR with this one. |
@fabioh8010 @rojiphil I think I addressed all your feedback! Anything else you find, let me know! 😄 |
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.
Minor comment
…al life conditions
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! @danieldoglas @rojiphil over to you
@pac-guerreiro Looks like we are marking the task as @danieldoglas My thoughts were that we have to mark it as Edit:
Sorry.. Just found this in the design doc. So, it is intended to complete the task once opened. Please ignore the above comment. |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari60085-web-chrome-001.mp4MacOS: Desktop60085-desktop-001.mp4Android: Native60085-android-hybrid-001.mp4Android: mWeb Chrome60085-mweb-chrome-001.mp4iOS: NativeNot tested as I have build issues. I think this is fine as the focus is not on mobile versions for this phase. iOS: mWeb Safari60085-mweb-safari-001.mp4 |
@pac-guerreiro There are conflicts here. Can you please resolve them? |
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.
@danieldoglas The only issues I notice are with the story lane test drive.
- There are issues with the mobile versions as mentioned here.
Curious to know if there is a plan to use swipe through carousel for mobile versions? I get this thought from the following statement in design document:
A limitation of this is that the mobile solution is not ideal as it’s not really a test drive, it’s just a swipe through carousel.
- Additionally, I have also left a comment here to improve the user experience.
Outside of the story lane though, the changes in this PR LGTM.
Approving for your review. Thanks.
…during-onboarding # Conflicts: # src/CONST.ts # src/libs/ReportUtils.ts
…during-onboarding
@rojiphil @fabioh8010 conflicts resolved! Thanks 😄 |
✋ 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 is looking pretty great to me. cc @Expensify/design (Would be good to get the design check before merging next time 😄 My bad for not being quick enough though) |
Same, looking good from what I can tell too 👍 |
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.1.33-0 🚀
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.1.35-1 🚀
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.1.36-3 🚀
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.1.37-1 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.37-3 🚀
|
Explanation of Change
Fixed Issues
$#60041
#60042
PROPOSAL: https://docs.google.com/document/d/1PryaYgnK8MeV2Zb_1Arp0HxSvRa7TJCjxGhTWGOVQ8s/edit?tab=t.0#heading=h.ntotu1k8frrn
Tests
Manage my team's expenses
1-10 employees
None of the above
Scenario A
Start test drive
Get started
on the top bar finishes the test driveScenario B
Take a test drive
taskGet started
on the top bar finishes the test driveOffline tests
Same as tests.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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.mp4
Android: mWeb Chrome
Android.-.Chrome.mp4
iOS: Native
iOS.-.Native.mp4
iOS: mWeb Safari
iOS.-.Safari.mp4
MacOS: Chrome / Safari
MacOS.-.Chrome.mp4
MacOS: Desktop
MacOS.-.Desktop.mp4