SignIn Screen, in SwiftUI #544
Conversation
krugerk
force-pushed
the
feature/swiftui-signin-screen
branch
from
cbd3487 to
9b7c83e
Compare
BeeSwift/SignInView.swift
Outdated
| } | ||
| .background(Color.gray) | ||
| .cornerRadius(8) | ||
| .disabled(viewModel.isLoading) |
There was a problem hiding this comment.
- black background, yellow border, yellow text when dark mode
- gray background, no border, black text when light mode
krugerk
force-pushed
the
feature/swiftui-signin-screen
branch
from
98bcd41 to
960796d
Compare
There was a problem hiding this comment.
Overall this looks good. Apple seems to be investing heavily enough in SwiftUI that it makes sense to start moving BeeSwift in that direction, and I agree the login screen is a good first candidate.
I added a bunch of comments inline. The ones tagged as [Do] are blocking for merge.
| try await Task.sleep(nanoseconds: 3_000_000_000) | ||
| } catch { | ||
| print("canceled early") | ||
| } |
There was a problem hiding this comment.
I think this is code you added to test the loading state but shouldn't be merged?
There was a problem hiding this comment.
The min process time was intentional so as to provide for a better user experience, actually. When tapping the button to trigger a sign in, it is usually a very quick process and one hardly has a chance to notice the progress indicator or the network activity indicator. If we are to show these, then they should be shown long enough for the user to have had a chance to recognize them.
There was a problem hiding this comment.
Plus when the user logs in, the endpoint returns a user object completed with a list of goal names. It could be that the app then presents a loading state of the gallery with the data it has while it continues fetching each goal and filling in the cells.
|
|
||
|
|
||
| private func presentSignInScreen() { | ||
| let shallPresentSignIn = presentedViewController == nil || !isSignInViewBeingPresented |
There was a problem hiding this comment.
[Nit] Isn't presentedViewController == nil duplicative with the rules in isSignInViewBeingPresented?
|
|
||
| let hostingVC = UIHostingController(rootView: SignInView()) | ||
| hostingVC.modalPresentationStyle = .fullScreen | ||
| self.present(hostingVC, animated: true) |
There was a problem hiding this comment.
No action: Medium term I'd like to move to having GalleryView only exist when logged in, and have a higher level view decide if the user is logged in or not, and present it or the sign in screen appropriately. That's definitely not something for this PR though.
BeeSwift/SignInButton.swift
Outdated
| .stroke(.clear) | ||
| } | ||
| } | ||
| // .cornerRadius(4) |
There was a problem hiding this comment.
[Nit] Either uncomment or delete
|
|
||
| var body: some View { | ||
| Button(action: action) { | ||
| Label("Sign In", systemImage: "iphone.and.arrow.forward.inward") |
There was a problem hiding this comment.
[Nit] I don't think this image actually renders anywhere?
|
|
||
| } | ||
| .alert("Could not sign in", isPresented: $showingFailedSignInAlert) { | ||
| Button("OK", role: .cancel) { } |
There was a problem hiding this comment.
How do these interact? Does clicking the OK button clear the variable?
| struct SignInView: View { | ||
| @StateObject private var viewModel = SignInViewModel() | ||
| @State private var showingFailedSignInAlert = false | ||
| @State private var showingMissingDataAlert = false |
There was a problem hiding this comment.
I'm curious how you decided what state should be in the ViewModel vs what should live directly in the view?
|
|
||
| Spacer() | ||
| Spacer() | ||
| Spacer() |
There was a problem hiding this comment.
[Nit] This is a surprising pattern. I'm guessing it's to result in the overall login being positioned vertically offset from center. I wonder if there is a way to do that directly via e.g. different weights for top and bottom padding? If not, let's add a comment.
| SecureField("Password", text: $viewModel.password) | ||
| .textFieldStyle(RoundedBorderTextFieldStyle()) | ||
| .textInputAutocapitalization(.never) | ||
| .submitLabel(.done) |
There was a problem hiding this comment.
[Consider] I confirmed using a password manager will auto-fill these fields. Can we make it auto-trigger the login after filling too?
| } | ||
| } | ||
|
|
||
| private func signIn() { |
There was a problem hiding this comment.
I would have guessed this method is allowed to be async (and thus not need the task)
took the opportunity to modernize the screen a bit background color of yellow, loading indicator and twaek the version shown in dark mode
krugerk
force-pushed
the
feature/swiftui-signin-screen
branch
from
960796d to
de9c2a4
Compare
Summary
The app has been bumped to min iOS 17 and much, if not all of the app, can be in SwiftUI. This MR can be a first step, the Sign In screen in SwiftUI.
screenshots of before and after
before
after
Validation
Notes
Would render #543 obsolete.