-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[NT-206] Reward received toggle business logic #866
Conversation
@@ -6,6 +6,8 @@ import UIKit | |||
final class ManagePledgeViewController: UIViewController { | |||
// MARK: - Properties | |||
|
|||
private let viewModel: ManagePledgeViewModelType = ManagePledgeViewModel() |
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've moved the viewModel
to the top of Properties section. This is consistent with the rest of the codebase and makes it easier to glance at the same place where we would in other VC
@@ -44,15 +46,6 @@ final class ManagePledgeViewController: UIViewController { | |||
|> \.translatesAutoresizingMaskIntoConstraints .~ false | |||
}() | |||
|
|||
private let viewModel: ManagePledgeViewModelType = ManagePledgeViewModel() |
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.
We've mostly used two step configuration
- Instantiate
- Configure
So I've gotten rid of this because it was effectively introducing pattern we've been trying to move away from and making the configureWith
function obsolete.
@@ -93,6 +96,9 @@ public final class ManagePledgeViewModel: | |||
self.goToChangePaymentMethod = self.menuOptionSelectedSignal | |||
.filter { $0 == .changePaymentMethod } | |||
.ignoreValues() | |||
|
|||
self.rewardReceivedViewControllerViewIsHidden = projectAndReward | |||
.map { project, reward in reward.isNoReward || project.personalization.backing?.status != .collected } |
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.
Hopefully this is the same logic @eoji was trying to explain to me ... please let me know if I'm off here
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.
:chefs_kiss: lgtm!
@@ -195,4 +199,160 @@ internal final class ManagePledgeViewModelTests: TestCase { | |||
self.goToUpdatePledgeProject.assertValues([Project.template]) | |||
self.goToUpdatePledgeReward.assertValues([Reward.template]) | |||
} | |||
|
|||
func testRewardReceivedViewControllerIsHidden_NoReward_Canceled() { |
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.
These tests could potentially only test when the view is NOT
hidden but since tests are cheap it should be not a problem for testing all the variations here.
Generated by 🚫 Danger |
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.
Looking good, minor comments, will test 👍
self.viewModel.outputs.rewardReceived | ||
.observeForUI() | ||
.observeValues { [weak self] isOn in | ||
self?.toggle.isOn = isOn |
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.
Didn't you add a RAC binding for this? self.toggle.rac.on
?
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.
Ah, got tripped by it being called on
not isOn
... yeah I can use it!
if AppEnvironment.current.device.userInterfaceIdiom == .pad { | ||
_ = nav | ||
_ = nc |
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.
The diff would probably be small with less renaming here 😬
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.
LGTM! 🚢
📲 What
Adds the business logic to
NoReward
, backing status !=collected
🤔 Why
Now that we have UI & a11y it's time to hook up the business logic
✅ Acceptance criteria
Default value
HIDE
reward received toggle
section if the project backing status isNOT
collected
HIDE
reward received toggle
section if thereward is
NoReward
SHOW
reward received toggle
section if both the project backing statusIS
collected
and the reward isNOT
aNoReward
(this could be verified against the backend by going to https://www.kickstarter.com/profile/backings?ref=user_menu and comparing the
Got it!
column for backed projects.Toggling value
(instead of navigating back and forth this could be verified against the backend by going to https://www.kickstarter.com/profile/backings?ref=user_menu