Manage pledge configuration outputs #838
Conversation
| @@ -3,15 +3,15 @@ import Library | |||
| import Prelude | |||
| import UIKit | |||
|
|
|||
| final class ManagePledgeViewController: UIViewController { | |||
| final class ManageViewPledgeViewController: UIViewController { | |||
There was a problem hiding this comment.
I think the old name is better 😬 what made you change it to ManageViewPledgeViewController?
There was a problem hiding this comment.
The main reason is that we will use the same screen to Manage and View pledge.
| @@ -93,33 +93,37 @@ public final class ProjectPamphletViewModel: ProjectPamphletViewModelType, Proje | |||
| .map(unpack) | |||
|
|
|||
| let goToManagePledge = ctaButtonTapped | |||
| .filter { canShowManageViewPledgeScreen($0.0, state:$0.2) } | |||
| .filter { canShowManageViewPledgeScreen($0.0, state: $0.2) } | |||
There was a problem hiding this comment.
The changes on this file were made by SwiftFormat.
| @@ -3,15 +3,15 @@ import Library | |||
| import Prelude | |||
| import UIKit | |||
|
|
|||
| final class ManagePledgeViewController: UIViewController { | |||
| final class ManageViewPledgeViewController: UIViewController { | |||
| public final class ManageViewPledgeViewModel: | ||
| ManageViewPledgeViewModelType, ManageViewPledgeViewModelInputs, ManageViewPledgeViewModelOutputs { | ||
| public init() { | ||
| let projectAndReward = self.projectAndRewardSignal |
There was a problem hiding this comment.
I think we usually use .combineLatest here to ensure that both signals have fired at least once.
There was a problem hiding this comment.
The problem with .combineLatest here is that will change the variable's type by combining the signals type. So instead of Signal<(Project, Reward), Never>, it would become Signal<((Project, Reward), Void), Never>. The .takeWhen guarantees that the .projectAndRewardSignal only emits when viewDidLoad emits without changing the original type. Does it make sense?
There was a problem hiding this comment.
It does change the signature, but that's not usually an issue because we can do .map(first).
I think problem with using .takeWhen is it that it doesn't wait for both signals to fire at least once 🤔 from the description of .takeWhen: "Emits the most recent value of selfwhenother emits.".
If the most recent of self (ie. projectAndRewardSignal) isn't there because that signal hasn't fired once, then it wouldn't work. Most of the time it would probably be fine, but we don't control when viewDidLoad gets called because it's a lifecycle function - I'm concerned there could be a race condition where somehow viewDidLoad gets called before we can call configureWith(project:reward).
cc @justinswart any feedback/insight on this?
There was a problem hiding this comment.
Yes, @ifbarrera is correct here in that we would want to use combineLatest because that negates the concern about the order in which these signals emit. Using takeWhen we must ensure that configure(with:) is called first so that it has a value to take when viewDidLoad emits. By combining the two we're allowed to call these inputs in any order. Our preference and the pattern we typically use for this is combineLatest 👍
There was a problem hiding this comment.
@Scollaco can you please make sure to make this change in a subsequent PR?
|
|
||
| self.vm.inputs.viewDidLoad() | ||
|
|
||
| self.title.assertValue(Strings.Manage_your_pledge()) |
There was a problem hiding this comment.
Can we use the actual values of these strings in here? It's better to be more explicit about what you're testing against:
self.title.assertValue("Manage your pledge")
|
|
||
| self.vm.inputs.viewDidLoad() | ||
|
|
||
| self.title.assertValue(Strings.View_your_pledge()) |
…com/kickstarter/ios-oss into manage-pledge-configuration-outputs
📲 What
Adds outputs on the
ManageViewPledgeViewControllerin order to configure each section (subview) separately.🤔 Why
This will avoid merge conflicts (or make them less bad) when more than one engineer start working on that screen.
🛠 How