-
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
Magic Link Login #1598
Magic Link Login #1598
Conversation
You can test the changes in simplenote-ios from this Pull Request by:
|
|
||
- vertical_whitespace | ||
# - vertical_whitespace |
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.
Disabling this, so that we stop chasing whitespaces or extra newlines, all over the place
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.
🙇
Overall this is looking good, going to wait to approve until we have the email link working (protocol not being stripped in gmail). Nice work Jorge! |
|
||
// MARK: - LoginRemote | ||
// | ||
class LoginRemote: Remote { |
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.
Is this that different from AccountRemote
? for the passkey remote things I dumped them in AccountRemote
I don't mind the specifically targeted class, but 🤔
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.
(Belayed reply!!) This one is indeed different from AccountRemote. Account encapsulates Email Verification + Account Deletion.
I'd be super okay with merging them both, though. I'll revisit in a follow-up, TY!!
|
||
// MARK: - MagicLinkRequestedView | ||
// | ||
struct MagicLinkRequestedView: View { |
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.
Currently almost all of the SwiftUI in SNiOS is done with UIKit. The only swiftUI we are using is in the widgets, where it is required. Do we want to start mixing UIKit and SwiftUI?
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.
Discussed over Slack, but leaving a note here, for the record!!
IMHO SwiftUI is all good, as long as it doesn't cause problems. That'd be: perhaps we should just try avoid blending SwiftUI + UIKit in the same UI, because of the (known) layout loops.
TY for bringing this up!!
|
||
/// Performs a URLSession Data Task | ||
/// | ||
func performDataTask(with request: URLRequest) async throws -> Data { |
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
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.
Looks good to me. I sent a few questions that we should ponder, but I'm not convinced that any changes need to happen based on those questions
Generated by 🚫 Danger |
Thank you Charlie!! We'll need to follow-up this PR, and also allow for Code Verification. Merging this one!! |
Details
This PR implements Magic Link Authentication support.
Ref. #1605
Test
Login
>Login with Email
Instantly Log In with Email
Log In
(in your email!), Simpenote logs you inLog In
button again, you won't get logged inTest: Regressions
Login with Email
Continue with Password
buttonForgot Password
opens the Password Recovery UI in a WebViewRelease
RELEASE-NOTES.txt
was updated in e1d5c04 with: