Add traitCollection input to LoginVM #959
Conversation
There was a problem hiding this comment.
Hey there @robjaq, thanks for your contribution! I hope you've found the codebase interesting to explore.
I've made some suggestions below and I will also look into the one failing job on our CircleCI, it looks like there is an environment variable missing in our configuration for builds running on forks.
| @@ -42,6 +42,9 @@ public protocol LoginViewModelInputs { | |||
|
|
|||
| /// Call when the view will appear. | |||
| func viewWillAppear() | |||
|
|
|||
| /// Call when a trait collection is applied. | |||
| func traitCollectionApplied() | |||
There was a problem hiding this comment.
We usually like to keep the input names as close as possible to the way it's named outside of the view model, so in this case it would be:
You can rename the input along with its backed properties, etc.
/// Call when a trait collection did change.
func traitCollectionDidChange()| @@ -171,7 +174,10 @@ public final class LoginViewModel: LoginViewModelType, LoginViewModelInputs, Log | |||
| self.logIntoEnvironment | |||
| .observeValues { _ in AppEnvironment.current.koala.trackLoginSuccess(authType: Koala.AuthType.email) } | |||
|
|
|||
| self.showHidePasswordButtonToggled = self.shouldShowPasswordProperty.signal | |||
| self.showHidePasswordButtonToggled = Signal.combineLatest( | |||
There was a problem hiding this comment.
In this case let's consider what we're wanting to express with the Signal. We're saying:
- Emit
showHidePasswordButtonToggledwhenshouldShowPasswordPropertyemits its value. - Also emit that same value when
traitCollectionDidChangeemits.
So by using comineLatest here, we're requiring that both of the signals emit in order for showHidePasswordButtonToggled to emit, which isn't really necessary. Instead I think what we want is to merge like this:
let shouldShowPassword = self.shouldShowPasswordProperty.signal
self.showHidePasswordButtonToggled = Signal.merge(
shouldShowPassword,
shouldShowPassword.takeWhen(self.traitCollectionDidChangeProperty.signal)
)This captures it a little more clearly by saying that we want to take the value of shouldShowPassword when self.traitCollectionDidChangeProperty.signal emits.
| @@ -314,6 +314,7 @@ final class LoginViewModelTests: TestCase { | |||
| func testShowPassword() { | |||
| self.vm.inputs.viewWillAppear() | |||
| self.vm.inputs.viewDidLoad() | |||
| self.vm.inputs.traitCollectionApplied() | |||
There was a problem hiding this comment.
After you make the previous change I suggested then you won't need this in this particular test because we will have changed the combineLatest to a merge.
Please add an additional test which tests the traitCollectionDidChange input.
|
@robjaq thanks for addressing that feedback. I have opened another PR #964 which, once we've merged into this one, should hopefully get all of our workflows passing on your branch. In the mean time, please run |
|
👌 |
|
@robjaq hey, please pull the latest from |
|
THat is the weekend! @justinswart Also feel free to merge it, since I do not have write access |
Wazzzzzzaaaap! :P Here is a proposed solution.
📲 What
Issue #949
🤔 Why
Eye icon and secure text loses its style during orientation change.
🛠 How
Add a trait collection input to the LoginViewModel and fire an event from the LoginVC for a trait collection change.
👀 See
Issue #949