-
Notifications
You must be signed in to change notification settings - Fork 168
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 Links: The one PR to rule them all #1205
Conversation
Magic Links: UI Updates Mk1
Authentication: Addressing Debt
…allback AuthenticationMode: RateLimiting Fallback
Co-authored-by: Jorge Leandro Perez <[email protected]>
Co-authored-by: Jorge Leandro Perez <[email protected]>
Magic Links: Fix window resizing animation + some clean up
Generated by 🚫 Danger |
You shall not pass… invalid login attempts! |
(Can be in follow-up PR) There's just a few capitalization issues that don't match the Figma:
|
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.
Tested all the steps and it worked great! Noted a few copy changes that can be added later, or here if you want!
GO MAGIC LINKS!
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.
✅ Code Auth works super
✅ Password Auth also works
✅ Signup is also good, just confirmed
✅ Rate Limited Login handled
✅ Invalid Code error, also, handled
✅ Transitions look super smooth, no Layout constraint errors onscreen
✅ Signup validation(s) also look good
✅ First Responder management also looks good, when you go back / forwards
Nice work Charlie!!
Fix
... one PR to find them, one PR to bring them all, and in the darkness bind them...
This PR brings together all of the previous changes to magic links authorization that has been made on SNMac and then merges them all back into trunk. There aren't any unreviewed changes here (although we might make some button copy changes before merging) but we should smoke test the auth views before we merge it back to trunk and then release the new auth to the masses.
fixes: #1187
Test
(Required) List the steps to test the behavior. For example:
Nothing specific here, but we want to test the new auth views so, try and consider this list of things:
Review
(Required) Add instructions for reviewers. For example:
Release
(Required) Add a concise statement to
RELEASE-NOTES.txt
if the changes should be included in release notes. Include details about updating the notes in this section. For example:Doesn't require updates because it is already there