SignIn Screen, in SwiftUI #544
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import SwiftUI | ||
|
|
||
| struct SignInButton: View { | ||
|
There was a problem hiding this comment. [Do] It looks like this isn't actually used. Let's either use it or delete it? |
||
| @Environment(\.colorScheme) private var colorScheme | ||
| let action: () -> Void | ||
| let isDisabled: Bool | ||
|
|
||
| init(action: @escaping () -> Void, isDisabled: Bool = false) { | ||
| self.action = action | ||
| self.isDisabled = isDisabled | ||
| } | ||
|
|
||
| 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? |
||
| .font(.system(size: 20)) | ||
| .frame(maxWidth: .infinity) | ||
| .frame(height: 44) | ||
| } | ||
| .buttonStyle(.bordered) | ||
| .foregroundStyle(colorScheme == .dark ? Color.yellow : Color.black) | ||
| .overlay { | ||
| if colorScheme == .dark { | ||
| RoundedRectangle(cornerRadius: 4) | ||
| .stroke(Color.yellow, lineWidth: 4) | ||
| } else { | ||
| RoundedRectangle(cornerRadius: 4) | ||
| .stroke(.clear) | ||
| } | ||
| } | ||
| // .cornerRadius(4) | ||
|
There was a problem hiding this comment. [Nit] Either uncomment or delete
krugerk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .disabled(isDisabled) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| // Part of BeeSwift. Copyright Beeminder | ||
|
|
||
| import SwiftUI | ||
| import BeeKit | ||
|
|
||
| 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? |
||
| @Environment(\.dismiss) private var dismiss | ||
|
|
||
| var body: some View { | ||
| ZStack { | ||
| Color.yellow | ||
| .opacity(0.8) | ||
| .ignoresSafeArea() | ||
|
There was a problem hiding this comment. [Do] I'm glad we're experimenting with design iterations. In this case I've looked at this in light mode and dark mode on device and simulator, and I think the full yellow background is too much. Let's stick with the current white/black background. |
||
|
|
||
| HStack(alignment: .center) { | ||
|
|
||
| VStack(alignment: .center, spacing: 15) { | ||
| Spacer() | ||
|
|
||
| Image("website_logo_mid") | ||
| .resizable() | ||
| .scaledToFit() | ||
| .frame(maxWidth: 400) | ||
| .padding(.bottom) | ||
|
There was a problem hiding this comment. [Consider] Matching the current design where the logo left/right matches the text inputs in portrait mode, i.e. it's less narrow than the full screen. |
||
|
|
||
| VStack(spacing: 15) { | ||
| TextField("Email or username", text: $viewModel.email) | ||
| .textFieldStyle(RoundedBorderTextFieldStyle()) | ||
| .textInputAutocapitalization(.never) | ||
| .autocorrectionDisabled() | ||
| .keyboardType(.emailAddress) | ||
| .textContentType(.emailAddress) | ||
| .submitLabel(.next) | ||
|
There was a problem hiding this comment. At present the placeholder text is too low contrast to be readable in dark mode. This might be resolved by changing the background though. |
||
|
|
||
| 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? |
||
|
|
||
| SignInButton(action: signIn, | ||
| isDisabled: viewModel.isLoading) | ||
| } | ||
| .frame(maxWidth: .infinity) | ||
| .padding(.horizontal, 40) | ||
|
|
||
| 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. |
||
| } | ||
| .padding() | ||
|
|
||
| } | ||
| .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? |
||
| } message: { | ||
| Text("Invalid credentials") | ||
| } | ||
| .alert("Incomplete Account Details", isPresented: $showingMissingDataAlert) { | ||
| Button("OK", role: .cancel) { } | ||
| } message: { | ||
| Text("Username and Password are required") | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: Notification.Name(CurrentUserManager.failedSignInNotificationName))) { _ in | ||
| viewModel.isLoading = false | ||
| showingFailedSignInAlert = true | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: Notification.Name(CurrentUserManager.signedInNotificationName))) { _ in | ||
| viewModel.isLoading = false | ||
| dismiss() | ||
| } | ||
| } | ||
| .overlay { | ||
| if viewModel.isLoading { | ||
| ProgressView() | ||
| .scaleEffect(1.5) | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| .background(Color.black.opacity(0.2)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func signIn() { | ||
|
There was a problem hiding this comment. I would have guessed this method is allowed to be |
||
| guard !viewModel.email.trimmingCharacters(in: .whitespaces).isEmpty, | ||
| !viewModel.password.isEmpty else { | ||
| showingMissingDataAlert = true | ||
| return | ||
| } | ||
|
|
||
| Task { | ||
| await viewModel.signIn() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| class SignInViewModel: ObservableObject { | ||
| @Published var email = "" | ||
| @Published var password = "" | ||
| @Published var isLoading = false | ||
|
|
||
| func signIn() async { | ||
| isLoading = true | ||
|
|
||
| do { | ||
| 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. There was a problem hiding this comment. I could maybe get behind this argument. However, I also log in/out a lot as part of testing the app, and waiting 3 seconds every time would be a significant frustration. I'd be open to having the login screen continue to show in the loading state until all goals are loaded into the gallery controller. But no explicit additional delays. |
||
|
|
||
| await ServiceLocator.currentUserManager.signInWithEmail( | ||
| email.trimmingCharacters(in: .whitespaces), | ||
| password: password | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| #Preview { | ||
| SignInView() | ||
| } | ||
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.
[Nit] Isn't
presentedViewController == nilduplicative with the rules inisSignInViewBeingPresented?