-
Notifications
You must be signed in to change notification settings - Fork 34
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
New Login Sheet #1064
New Login Sheet #1064
Conversation
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! A couple minor notes about naming, hashing, and consistency, and I've found a few minor defects:
- The flow does not check if the account already exists before adding it.
- Trying to connect to an invalid instance locks up the app until it resolves, which could be upwards of 30 seconds, and doesn't offer any feedback on failure. There should be a timeout (5s?) and a "connection timed out" notice.
On a UX note, I think selecting an instance from the dropdown should auto-submit "next"--it's very easy to go back and change if the user taps the wrong instance, and makes the common case a tap faster.
Mlem/App/Views/Shared/Navigation/EnvironmentValue+IsFirstPage.swift
Outdated
Show resolved
Hide resolved
If the account already exists, it will update the token of the existing account rather than trying to add a second one. Because |
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
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.
LGTM! 🔥
This PR implements the login sheet (including two-factor authentication) and the token refresh sheet. For testing purposes, a "Re-authenticate" button has been added to the account list in settings. The token refresh sheet is not yet invoked automatically when the token expires.
Important
This PR is dependent on the changes made in this PR and the changes made in this PR.
Login flow:
Token refresh sheet, which will also show the 2FA page if needed:
The syntax for opening the login sheet is as follows:
Closes #1057