-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Replace native WP.com sign in with web-based sign in #23675
base: trunk
Are you sure you want to change the base?
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
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 tested the flow, including logging-in with a new account in the web view and using passkeys. The flow works well.
It is a bit of a downgrade compared to the current implementation where the app prompts you to use one of the saved credentials from the Apple Passwords on first launch. Perhaps web views should be reserved for self-hosted sites and for wp.com account creation.
} else { | ||
wpAssertionFailure("WP.com web login failed", userInfo: ["error": "\(error)"]) | ||
} | ||
return |
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.
(nit) Extract the first part into a separate method so that there is no return
in the middle of this long method.
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.
Addressed in 43409de
// TODO: Replce with a remote feature flag. | ||
// Enable web-based login for debug builds until the remote feature flag is available. | ||
#if DEBUG | ||
let webLoginEnabled = true |
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.
If there is no need to update the flag remotely, you can use extension FeatureFlag: RolloutConfigurableFlag {
to implement the percentage-based rollout locally.
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.
In that case, we'd need to release the app to update the rollout percentage, 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.
Yup
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.
Personally, I'd prefer a remote feature flag because we can completely turn it off if there are horrible issues.
func signIn(from viewController: UINavigationController) async { | ||
let token: String | ||
do { | ||
token = try await authenticate(from: viewController) |
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.
The animation at the top is distracting.
Untitled.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.
I'm not sure why it looks like this for me. It's different on your recording. I think there is a bug with the first welcome page.
The web based sign-in flow is enabled on debug builds. But it will be hidden under a feature flag later.
sign-in.mp4
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: