-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[WooCommerce] Fix inconsistent social login screen when email matches an existing account #87073
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~14867 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~43191 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~50411 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
8897fcd
to
cc14226
Compare
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 tried following the testing instructions but at step 6 instead of observing the notification about having an existing account, it just sent me to the woo dashboard with the account created for me.
This is with a google account that has never been used for wpcom nor wccom.
While this is unexpected from the testing instructions, I don't actually see something wrong with this outcome?
b1ade37
to
d56b81e
Compare
cc14226
to
334041b
Compare
@rjchow So there are no existing wpcom accounts matching this Google account's email? If that's the case, then there's no problem. If you create a wpcom account using the email signup and then try to log in with the Google account associated with that email, you should see an existing account notice. 🤔 |
Oh cool! i misunderstood the instructions 😅 I’ll try it again shortly with the new understanding! |
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.
Yep can confirm I get the woo lost password screen after seeing the existing account dialog. It seems that the target audience for this fix will certainly see this screen if they signed up via passwordless, which is the default wpcom sign up now.
The send me a login link
is not working correctly it seems, there's no href in the :
Would it be possible to fix this in this PR too?
@chihsuan I don't get the step #7 here, when does "Check your email" screen supposed to appear? I tried forgot password and entered my email which is disconnected from Google account, it doesn't show the screen either: |
Hey @ilyasfoo, Sorry, it seems that actually only passwordless accounts (sign up through wp.com) will enter the magic link flow and the "Check your email" screen will appear automatically. Could you try again with a passwordless account? You could check if an account is passwordless or not by running wp-calypso/client/blocks/login/index.jsx Lines 143 to 145 in a445602
|
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.
Hey @ilyasfoo, Sorry, it seems that actually only passwordless accounts (sign up through wp.com) will enter the magic link flow and the "Check your email" screen will appear automatically. Could you try again with a passwordless account?
Got it, thanks! I've confirmed this to work with a new passwordless account.
I'm not sure this is related, but putting it here: It seems the padding/margin before the prompt is missing when I try to login using new Google account:
Otherwise, this is testing well, pre-approving!
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.
@chihsuan Awesome, fix tests well too, thanks! 👍
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.
Thank you very much! LGTM 💯
Closes #86885
Proposed Changes
Testing Instructions
You already have a WordPress.com account with this email address. Add your password to log in or create a new account.
Pre-merge Checklist