[MBL-1755] Pledge Summary Table Total Amount Text #2163
Conversation
scottkicks
requested review from
a team,
jovaniks,
ifosli and
stevestreza-ksr
and removed request for
a team and
jovaniks
There was a problem hiding this comment.
Added a couple comments; I'm confused by why the "no reward" bool is separate from the rest of the config data and I'm missing a snapshot test for crowdfunding checkout. Would you please fix/explain?
Also, since this string is longer, would you mind double-checking what the UI looks like for large font sizes or maybe in german? UI doesn't need to be perfect but it should be usable.
| @@ -10,7 +10,7 @@ public typealias PledgeSummaryViewData = ( | |||
| ) | |||
There was a problem hiding this comment.
Can we add the pledgeHasNoReward bool here instead of passing it in separately?
There was a problem hiding this comment.
Kind of outside the scope of this pr, but do we still need separate post campaign and crowdfunding versions of this table?
There was a problem hiding this comment.
I'm refactoring this table in my current ticket. This should make it easier for these classes to use one version of this table.
There was a problem hiding this comment.
These snapshots look great! Will you also add snapshot tests for the no rewards case for crowdfunding (assuming we don't have one already; if we do, will you look into what's going on and fix it)?
| .map { project, confirmationLabelHidden, total in (project, total, confirmationLabelHidden) } | ||
| .map { projectAndConfirmationLabelHidden, pledgeTotal, rewards in | ||
| let (project, confirmationLabelHidden) = projectAndConfirmationLabelHidden | ||
| let pledgeHasNoReward = rewards.count == 1 && rewards.first?.isNoReward == true |
There was a problem hiding this comment.
Nit: Could use pledgeHasNoRewards(rewards:) here
| .map { project, confirmationLabelHidden, total in (project, total, confirmationLabelHidden) } | ||
| .map { projectAndConfirmationLabelHidden, pledgeTotal, rewards in | ||
| let (project, confirmationLabelHidden) = projectAndConfirmationLabelHidden | ||
| let pledgeHasNoReward = rewards.count == 1 && rewards.first?.isNoReward == true |
There was a problem hiding this comment.
Same nit: Could use pledgeHasNoRewards(rewards:) here
| @@ -211,7 +211,7 @@ private func items( | |||
|
|
|||
| // MARK: Bonus | |||
|
|
|||
| if let bonus = bonusAmount, bonus > 0 { | |||
| if let bonus = bonusAmount, bonus > 0, rewardItems.isEmpty == false { | |||
There was a problem hiding this comment.
Sanity checking this shouldn't be doing that same isNoReward check here
There was a problem hiding this comment.
either or would work here actually.
scottkicks
dismissed
ifosli’s stale review
Need to get this merged in and Ingerid is OOO
📲 What
We also should be showing “Pledge without a reward” when a user pledges without a reward, instead of “Bonus support” and the total “Pledge amount”
🛠 How
This section of the pledge summary is managed by
NoShippingPledgeRewardsSummaryTotalViewControllerfor crowdfunding checkout andPostCampaignPledgeRewardsSummaryTotalViewControllerfor late pledge checkout.We need these classes to know if the backer is pledging without a reward so they can display either “Pledge without a reward” or “Pledge amount” so, I'm passing the
baseReward, which has a handyisNoRewardhelper, from the view model through to these classes.The designs also show that, when pledging without a reward, the bonus support and reward table view rows should be hidden.
👀 See
Designs for reference
✅ Acceptance criteria