-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: authentication flow view -> Cloud password login - WPB-15222 #2455
base: develop
Are you sure you want to change the base?
feat: authentication flow view -> Cloud password login - WPB-15222 #2455
Conversation
Test Results 2 files 12 suites 47s ⏱️ Results for commit 7defe15. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 102 Passed, 1 Skipped, 17.01s Total Time |
@KaterinaWire LGTM, left a few non blocking comments, also I don't see the second view ("don't have a wire account..") in the snapshots |
WireUI/Sources/WireReusableUIComponents/LabeledTextField/LabeledTextField.swift
Outdated
Show resolved
Hide resolved
WireUI/Sources/WireReusableUIComponents/PasswordField/PasswordField.swift
Show resolved
Hide resolved
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 work!
WireUI/Sources/WireAuthenticationUI/Views/LoginViaEmailView.swift
Outdated
Show resolved
Hide resolved
WireUI/Sources/WireAuthenticationUI/ViewModels/LoginViaEmailViewModel.swift
Outdated
Show resolved
Hide resolved
WireUI/Sources/WireAuthenticationUI/Views/LoginViaEmailView.swift
Outdated
Show resolved
Hide resolved
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 work! We should probably take a look at some of our access modifiers. Most of them don't need to be public
- package
should be enough.
WireAuthentication/Sources/WireAuthentication/Components/RootComponent.swift
Outdated
Show resolved
Hide resolved
...entication/Sources/WireAuthenticationUI/Views/Preview/SwitchBackendConfirmationPreview.swift
Outdated
Show resolved
Hide resolved
@MainActor var accountsURL: URL { get } | ||
@MainActor var passwordValidator: any PasswordValidator { get } |
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.
question: why do these need to marked @MainActor
?
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.
don't need, fixed
public let accountsURL: URL | ||
public let passwordValidator: any PasswordValidator | ||
|
||
init(accountsURL: URL, passwordValidator: any PasswordValidator) { |
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.
question: does the password validator need to be injected because it the rules to validate a password are configurable and come from the Wire-iOS project?
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.
Yes.
We have a protocol
public protocol PasswordValidator {
func validate(_ password: String) -> Bool
var localizedRulesDescription: String? { get }
}
which will be reused for export/import backup.
|
||
@ObservedObject var viewModel: LoginOrRegisterViaEmailViewModel | ||
|
||
var body: some View { | ||
Color.red | ||
@State var password: String = "" |
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.
question: I wonder if this should be part of the view model?
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.
I would keep it as a private
in the view.
- We don't need to pass
password
to the multiple views (as it's with email), so there is no need to store it. - From the performance point of view, changing the password only updates this view, not the entire
ViewModel
.
} | ||
|
||
public var body: some View { | ||
VStack(alignment: .center, spacing: 14) { |
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.
suggestion: improve readability by breaking up a large body
into components like:
var body: some View {
VStack {
emailField
passwordField
submitButton
forgotPasswordButton
}
}
@ViewBuilder
private var emailField: some View (
LabeledTextField(
placeholder: nil,
title: L10n.CloudUserLogin.InputEmail.title,
string: .constant(viewModel.email
)
.disabled(true)
}
@ViewBuilder
private var passwordField: some 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.
done
.disabled(!viewModel.isValidPassword(password)) | ||
|
||
Button(action: { | ||
UIApplication.shared.open(viewModel.forgotPasswordURL) |
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.
I think this should be done in the view model:
viewModel.recoverPassword()
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.
done
} | ||
|
||
func submitPassword(_ password: String) { | ||
Task { |
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.
suggestion: run this in a detached task, otherwise it will run on the main thread and block the UI.
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.
done
@ObservedObject var viewModel: LoginViaEmailViewModel | ||
|
||
@State private var password = "" | ||
@State var password: String = "" |
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.
suggestion: move this to the view model. If you keep in here, it should be marked as private
(SwiftUI best practice)
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.
made it private
TextField("", text: .constant(viewModel.email)) | ||
.textFieldStyle(.roundedBorder) | ||
.foregroundStyle(.secondary) | ||
ScrollView { |
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.
suggestion: increase readability by breaking this up into components and using view builders.
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.
done
let forgotPasswordURL: URL | ||
let passwordValidator: any PasswordValidator |
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.
suggestion: mark everything except email
private.
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.
done
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.
I know I was the one to separate LoginViaEmail
and LoginOrRegisterViaEmail
, but i wonder if it would make sense to combine them since they are almost identical. What do you think?
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.
@johnxnguyen I would keep it separately. I see some Pros
and Cons
, but maybe we can keep it separately for now.
Cons:
- duplicate code
Pros:
- view model for
LoginOrRegisterViaEmail
contains additional methods, that should be nil for login without registration; - single responsibility;
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.
But we will have one more similar view and will increase code duplication. I combined LoginViaEmail
and LoginOrRegisterViaEmail
Issue
This PR introduces 2 views for Path 1, 2 and 3, but to fit the design, changes need to be made for 2 reusable elements::
PasswordField
andLabeledTextField
. As a result of these changes, we need to update Snapshots.Testing
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: