[NT-435] - Pledge maximum adding shipping rules #911
Conversation
Scollaco
changed the title
| @@ -68,6 +68,8 @@ public protocol PledgeViewModelOutputs { | |||
| var submitButtonIsLoading: Signal<Bool, Never> { get } | |||
| var submitButtonTitle: Signal<String, Never> { get } | |||
| var title: Signal<String, Never> { get } | |||
| var updateMaximumPledgeAmount: Signal<Double, Never> { get } | |||
| var updatePledgeFailedWithError: Signal<String, Never> { get } | |||
There was a problem hiding this comment.
Was this output re-added during a merge? I believe it's now removed on master.
| @@ -156,6 +160,10 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge | |||
| let pledgeAmountIsValid = self.pledgeAmountDataSignal | |||
| .map { $0.isValid } | |||
|
|
|||
| pledgeAmountIsValid.observeValues { v in | |||
There was a problem hiding this comment.
Used for debugging purposes and missed it during cleanup. 🤦♂
| project, | ||
| self.pledgeAmountDataSignal | ||
| ) | ||
| .takeWhen(Signal.merge(showApplePayAlert)) |
There was a problem hiding this comment.
Do we still need this .merge?
| @@ -575,6 +583,8 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge | |||
|
|
|||
| self.notifyDelegateUpdatePledgeDidSucceedWithMessage = updateBackingCompletionEvents | |||
| .mapConst(Strings.Got_it_your_changes_have_been_saved()) | |||
| self.updatePledgeFailedWithError = updateBackingEvent.errors() | |||
There was a problem hiding this comment.
I think this is removed on master.
| @@ -15,6 +15,7 @@ public protocol PledgeAmountViewModelInputs { | |||
| func stepperValueChanged(_ value: Double) | |||
| func textFieldDidEndEditing(_ value: String?) | |||
| func textFieldValueChanged(_ value: String?) | |||
| func updateMaximumPledgeAmount(with shippingCost: Double) | |||
There was a problem hiding this comment.
The input should be named based on the event that occurred, so in this case I think it's selectedShippingAmountChanged(to amount: Double).
| let maxValue = minAndMax | ||
| .map(second) | ||
| .combineLatest(with: shippingCost) | ||
| .map { max, shippingCost in |
There was a problem hiding this comment.
Swift is so fancy this can actually be .map(-) 😄
| @@ -156,6 +160,10 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge | |||
| let pledgeAmountIsValid = self.pledgeAmountDataSignal | |||
| .map { $0.isValid } | |||
|
|
|||
| pledgeAmountIsValid.observeValues { v in | |||
| @@ -706,6 +716,8 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge | |||
| public let submitButtonIsLoading: Signal<Bool, Never> | |||
| public let submitButtonTitle: Signal<String, Never> | |||
| public let title: Signal<String, Never> | |||
| public let updateMaximumPledgeAmount: Signal<Double, Never> | |||
| public let updatePledgeFailedWithError: Signal<String, Never> | |||
| @@ -156,6 +163,10 @@ final class PledgeAmountViewController: UIViewController { | |||
| @objc func textFieldDidChange(_ textField: UITextField) { | |||
| self.viewModel.inputs.textFieldValueChanged(textField.text) | |||
| } | |||
|
|
|||
| func selectedShippingAmountChanged(to amount: Double) { | |||
There was a problem hiding this comment.
Can we put this under // MARK: - Accessors
| @@ -117,6 +118,8 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge | |||
|
|
|||
| let pledgeTotal = Signal.combineLatest(pledgeAmount, shippingCost).map(+) | |||
|
|
|||
| self.selectedShippingAmountChanged = shippingCost | |||
There was a problem hiding this comment.
So typically we'd want this sort of information to be sent via a delegate but in this case our PledgeViewController will have two delegates 🤔. I think this is a bit of an anti-pattern and points to a larger architectural problem that we have with our VCs talking directly to each other. I have thoughts on how we can improve this but it's outside the scope of this work.
In any case, I would suggest at least renaming this output to make what this is doing a bit clearer - something like notifyPledgeAmountViewControllerShippingAmountChanged.
We're likely going to want to do some refactoring here down the line but I don't want to hold this up to do that right now.
Please make this change before merging though.
There was a problem hiding this comment.
Yes, I agree with that and I think it will be a fun problem to solve. A discussion in this topic with the team would be worth it. But for now, thanks for you input 😄
📲 What
🤔 Why
JIRA ticket
🛠 How
PledgeAmountViewModeland updating the max pledge by subtracting the currency's max pledge and shipping costPledgeAmountViewControllerthat appears whenever the amount is greater than the new max pledge amount👀 See
♿️ Accessibility
✅ Acceptance criteria
Project with no Shipping Rules:
Please enter a pledge amount between [reward min] and [currency max pledge].Donebutton should be `disabled.Project with Shipping Rules:
currency max pledge amount(e.g $10000 for US projects) minusshipping Cost. Then select a new shipping rule with greater shipping cost. A label should appear with the textPlease enter a pledge amount between [reward min] and [new max]Donebutton should be `disabled.