-
Notifications
You must be signed in to change notification settings - Fork 285
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
Login: Auth Code Interface #1615
Conversation
Generated by 🚫 Danger |
You can test the changes in simplenote-ios from this Pull Request by:
|
return false | ||
} | ||
} | ||
|
||
|
||
// MARK: - State |
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.
All of the structs below have been extracted + simplified into AuthenticationMode
@@ -850,50 +850,6 @@ private enum PasswordInsecureString { | |||
} | |||
|
|||
|
|||
// MARK: - Mode: .loginWithPassword | |||
// | |||
private enum PasswordStrings { |
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.
Same note as above: extracted into AuthenticationMode
break | ||
} | ||
|
||
performPrimaryActionIfPossible() |
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.
performPrimaryActionIfPossible
is now responsible for rendering Warnings, when / if needed.
Plus we'll never show Username + Password fields, at once, so we're simplifying this
@@ -662,6 +756,7 @@ private extension SPAuthViewController { | |||
func ensureWarningsAreOnScreenWhenNeeded() -> Bool { | |||
let usernameValidationResult = performUsernameValidation() | |||
let passwordValidationResult = performPasswordValidation() | |||
let codeValidationResult = performCodeValidation() |
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.
Extending pre-existant validation mechanisms, to check for the Code as well
SPTracker.trackLoginLinkConfirmationFailure() | ||
self.handleError(error: error) | ||
} catch { | ||
// TODO: Fixme |
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.
To be revisited in a follow up:
- I'm not fond of not having typed errors
- I think rate limited responses should be handled in a specific way (ie. pushing the Password UI)
lockdownInterface() | ||
|
||
controller.requestLoginEmail(username: email) { error in | ||
// TODO: 429 (Rate Limited)? push PW auth instead |
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.
To be handled in a follow up. Rate limiting errors should prob. cause the Password UI to show up
@@ -332,16 +381,21 @@ private extension SPAuthViewController { | |||
|
|||
// MARK: - Actions | |||
// | |||
private extension SPAuthViewController { | |||
extension SPAuthViewController { |
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.
Removing private
since the AuthenticationMode descriptors live in its own file
|
||
actionView.addTarget(self, action: descriptor.selector, for: .touchUpInside) | ||
actionView.isHidden = false | ||
} |
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.
Ideally we don't have 4x Actions, and use them accordingly. Instead, we should simply instantiate the UIButton
instances, based on the descriptor.
I'm keeping the changeset delta on check, though, and following along. For now!
|
||
/// # Primary Action: LogIn / SignUp | ||
/// | ||
@IBOutlet private var primaryActionButton: SPSquaredButton! { | ||
didSet { | ||
primaryActionButton.setTitle(mode.primaryActionText, for: .normal) |
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.
Most of the UI initialization code that was deleted here, will be handled by the new API(s) refreshActionViews
and reloadInputViewsFromState
.
|
||
return usernameValidationResult == .success && passwordValidationResult == .success | ||
return usernameValidationResult == .success && passwordValidationResult == .success && codeValidationResult == .success |
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 duplicated in L192. Should it be extracted somewhere?
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 auth view is much more complex now, but this does a good job of simplifying additions and changes.
Yeahhhh I'm tempted to just start splitting (or even going SwiftUI), but with this approach we can add or remove things in a split second. Adding Passkeys should take a minute, now? Thank you Charlie!! |
Details
In this PR we're:
AuthenticationMode
into its own fileAuthViewController
initialization, so that it uses dynamic descriptorsCode
fieldRef. #1611
Test: Login with Code
Log In
Log in with email
Test: Login with Password
Log In
Log in with email
Enter Password
Log In
gets you a warning onscreenTest: Login with WP
Log In
Log in with WordPress.com
Test: Signup
Signup
Sign Up
without entering a valid email gets you a warning onscreenRelease