-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Update mobile app onboarding modal to be two steps #40613
Conversation
… used in Calypso.
…ep to install the app.
Test Results SummaryCommit SHA: 21094a9
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
…ing-task-iteration-2 * wcios/mobile-onboarding-task: Fit mobile app modal height to content with a max height that's taller than the default modal guide. Replace mobile static QR code SVG with `QRCodeSVG`. Add `qrcode.react` for a React component that displays a QR code also used in Calypso.
Hi @rjchow, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
@rjchow @coreymckrill This is the follow-up of the previous PR regarding the onboarding banner. It's a bigger one but quite important to achieve our objectives with the app login flow. So I'm requesting the review from you both since you folks reviewed the last one. Thank you in advance 🙏 |
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.
Left some feedback!
}: { | ||
loginUrl: string | undefined; | ||
} ) => { | ||
// TODO: update the minimum app version |
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.
😬
...ns/woocommerce-admin/client/homescreen/mobile-app-modal/components/MobileAppLoginStepper.tsx
Show resolved
Hide resolved
} else { | ||
const siteUrl: string = getAdminSetting( 'siteUrl' ); | ||
const username = getAdminSetting( 'currentUserData' ).username; | ||
const loginUrl = `woocommerce://app-login?siteUrl=${ encodeURIComponent( |
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 was implemented a bit differently than I had imagined, I was thinking that we could add some code to wc core that can generate the application password, which would be triggered by the user opening this task.
When this modal opens, the QR should already include the application password so that there's no further need for any credentials to be provided by the user.
Furthermore, I was thinking that there should be a deeplink that immediately brings the user to install woocommerce mobile app if they don't already have it, and then when it is installed the app should also have the application password forwarded to it. This would lead to a true seamless experience
Are these part of the future roadmap?
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.
Hi @rjchow 👋
This was implemented a bit differently than I had imagined, I was thinking that we could add some code to wc core that can generate the application password, which would be triggered by the user opening this task.
This is iteration 2 of the improvements. We have on our radar iteration 3 https://github.com/woocommerce/woocommerce/pull/40628/files, which will include the application password and is closer to what we already discussed.
Furthermore, I was thinking that there should be a deeplink that immediately brings the user to install woocommerce mobile app if they don't already have it, and then when it is installed the app should also have the application password forwarded to it. This would lead to a true seamless experience
💯 agree with you! We are still looking at how to support deferred deep links in the app, but I agree this is the way to go
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.
Ok great to know that! Thanks!
<QRCodeSVG value={ loginUrl } size={ 140 } /> | ||
<p> | ||
{ __( | ||
'The app version needs to be 15.7 or above to sign in with this link.', |
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 realised that I can't test this flow because the latest version I can install is 15.6
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 realised that I can't test this flow because the latest version I can install is 15.6
@rjchow Can you try installing the apps from these PR? :
- If you own an iOS device: [Onboarding Deep Link] Add login deep link track events woocommerce-ios#10892 (comment)
- If you own an Android device: App login deep link track events woocommerce-android#9920 (comment)
If you need any help, please ping me. I'll be more than happy to support you.
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 went on with it for a bit then realised that I need to install app center device profile on my personal phone so I noped outta there 😅 I'll take it that it works!
Fixed here a4c3834
Fixed here 2d1dec8
Fixed here 6f9f49a Thanks for your review @rjchow! I fixed all the issues. I think we are ready for a second round 😄 |
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.
Nice work, pre-emptively approving! If you need me to merge it just ping me
} else { | ||
const siteUrl: string = getAdminSetting( 'siteUrl' ); | ||
const username = getAdminSetting( 'currentUserData' ).username; | ||
const loginUrl = `woocommerce://app-login?siteUrl=${ encodeURIComponent( |
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.
Ok great to know that! Thanks!
plugins/woocommerce-admin/client/homescreen/mobile-app-modal/style.scss
Outdated
Show resolved
Hide resolved
<QRCodeSVG value={ loginUrl } size={ 140 } /> | ||
<p> | ||
{ __( | ||
'The app version needs to be 15.7 or above to sign in with this link.', |
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 went on with it for a bit then realised that I need to install app center device profile on my personal phone so I noped outta there 😅 I'll take it that it works!
…tyle.scss Co-authored-by: RJ <27843274+rjchow@users.noreply.github.com>
Thanks for your review @rjchow!! 🙏
Yes, that would be great. We have no permission to merge to trunk. |
This isn't one of the critical flows so I'm merging this without additional QA review |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes https://github.com/woocommerce/woomobile-private/issues/315
Example screenshots
login-link-non-jetpack-site.mov
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
TC1
Prerequisite: the site has the the full Jetpack plugin (WPCOM sites have Jetpack by default), and hasn't completed all onboarding tasks. The connected WPCOM user is not linked to an A8C email (magic links don't work for A8C emails)
TC2
Non-Jetpack sites
Prerequisite: the site doesn't have the Jetpack plugin
Changelog entry
Significance
Type
Message
Comment