[MBL-1208] Part 1: Initial ConfirmDetailsViewController | Shipping Location + Pledge/Bonus Steppers #1969
Conversation
scottkicks
changed the title
scottkicks
force-pushed
the
scott/pcp/initial-confirm-details-controller
branch
from
e7ad4d4
to
47cc8a9
Compare
| private lazy var rootScrollView: UIScrollView = { | ||
| UIScrollView(frame: .zero) | ||
| |> \.translatesAutoresizingMaskIntoConstraints .~ false | ||
| }() | ||
|
|
||
| private lazy var rootStackView: UIStackView = { | ||
| UIStackView(frame: .zero) | ||
| |> \.translatesAutoresizingMaskIntoConstraints .~ false | ||
| }() | ||
|
|
||
| private lazy var rootInsetStackView: UIStackView = { | ||
| UIStackView(frame: .zero) | ||
| |> \.translatesAutoresizingMaskIntoConstraints .~ false | ||
| }() |
There was a problem hiding this comment.
Setup for the sections being added in the next 2 PRs. (Pledge summary table and CTA)
scottkicks
force-pushed
the
scott/pcp/initial-confirm-details-controller
branch
from
47cc8a9
to
a32f3ab
Compare
The initial view controller includes: * Shipping location selector (when available) * Pledge/Bonus stepper options (when available)
scottkicks
force-pushed
the
scott/pcp/initial-confirm-details-controller
branch
from
a32f3ab
to
f693c41
Compare
scottkicks
requested review from
a team and
amy-at-kickstarter
and removed request for
a team
Generated by 🚫 Danger |
scottkicks
changed the title
| private func pledgeSummaryViewData( | ||
| project: Project, | ||
| total: Double, | ||
| confirmationLabelHidden: Bool | ||
| ) -> PledgeSummaryViewData { | ||
| return (project, total, confirmationLabelHidden) | ||
| } | ||
|
|
||
| private func pledgeAmountSummaryViewData( | ||
| with project: Project, | ||
| reward _: Reward, | ||
| allRewardsTotal: Double, | ||
| additionalPledgeAmount: Double, | ||
| shippingViewsHidden: Bool, | ||
| context: PledgeViewContext | ||
| ) -> PledgeAmountSummaryViewData? { | ||
| guard let backing = project.personalization.backing else { return nil } | ||
|
|
||
| let rewardIsLocalPickup = isRewardLocalPickup(backing.reward) | ||
| let projectCurrencyCountry = projectCountry(forCurrency: project.stats.currency) ?? project.country | ||
|
|
||
| return .init( | ||
| bonusAmount: additionalPledgeAmount, | ||
| bonusAmountHidden: context == .update, | ||
| isNoReward: backing.reward?.isNoReward ?? false, | ||
| locationName: backing.locationName, | ||
| omitUSCurrencyCode: project.stats.omitUSCurrencyCode, | ||
| projectCurrencyCountry: projectCurrencyCountry, | ||
| pledgedOn: backing.pledgedAt, | ||
| rewardMinimum: allRewardsTotal, | ||
| shippingAmount: backing.shippingAmount.flatMap(Double.init), | ||
| shippingAmountHidden: !shippingViewsHidden, | ||
| rewardIsLocalPickup: rewardIsLocalPickup | ||
| ) | ||
| } |
There was a problem hiding this comment.
These are actually for the next pieces. Forgot to remove these for this initial PR
| self.viewModel.outputs.configureShippingLocationViewWithData | ||
| .observeForUI() | ||
| .observeValues { [weak self] data in | ||
| self?.shippingLocationViewController.configureWith(value: data) |
There was a problem hiding this comment.
Nit: the other methods are configure(with:) and this is configureWith(value:)
| .observeValues { [weak self] data in | ||
| guard let self else { return } | ||
|
|
||
| if featurePostCampaignPledgeEnabled(), data.project.isInPostCampaignPledgingPhase { |
There was a problem hiding this comment.
Nit: If this double-check (for the feature and the project flag) becomes used more frequently in the app, it might be nice to have a helper method like usePostCampaignPledgeFlow(forProject: project)
| target: self, | ||
| action: #selector(ConfirmDetailsViewController.dismissKeyboard) | ||
| ) | ||
| |> \.cancelsTouchesInView .~ false |
There was a problem hiding this comment.
Nit: I would prefer to see these done without the use of Prelude/lenses, just because I have a sneaking suspicion (based on some suspicious timing output in Xcode) that the the use of many Prelude operators leads to slow compile times. Using lenses for mutable properties like cancelsTouchesInView is unecessary.
However, I agree that being expedient is the most important thing here, so if this is the fastest way for you to do this - I assume you're working off an example elsewhere in the app - ignore me.
| /** | ||
| Shipping summary view is hidden when updating, | ||
| if the base reward has no shipping, when NO add-ons were selected or when base reward has local pickup option. | ||
| */ |
There was a problem hiding this comment.
A+ comments here in the VM
| self.configureWithDataProperty.signal, | ||
| self.viewDidLoadProperty.signal | ||
| ) | ||
| .map(first) |
There was a problem hiding this comment.
Nit: This is a pattern we use everywhere, but every time I see us using first, I assume this is a static/unchanging value, and should really just be passed in on init for the view model. However, again, I assume this is more expedient to write since you're following established patterns, so this is non-blocking.
| (projectAndReward.0, projectAndReward.1, selectedLocationId) | ||
| } | ||
| .map { project, reward, locationId in | ||
| (project, reward, true, locationId) |
There was a problem hiding this comment.
Nit: I've been finding the use of large tuples in checkout to be confusing. Could this be a small wrapper object, instead?
| initialAdditionalPledgeAmount | ||
| ) | ||
| .map(unpack) | ||
| .map { project, reward, additionalPledgeAmount in |
There was a problem hiding this comment.
Is this unpack/map necessary? Seems to be outputting the same values it started with.
| } | ||
|
|
||
| // Only shown for if the shipping summary view and shipping location view are hidden | ||
| self.configureLocalPickupViewWithData = Signal.combineLatest( |
There was a problem hiding this comment.
Nit: Seems like a lot of this VM is for local pickup/shipping issues. Do we do any kind of view/vm composition for issues like this? Feels like it could be logically separate, in many ways, but I'm not sure I've seen us doing that elsewhere in the codebase. I think you'd end up with smaller/clearer components if you were able to subdivide this out.
|
@amy-at-kickstarter Thanks for your comments. Your re-structure suggestions are super valid. I think that it's ok to move on with this for now though. I want this flow to be an early candidate for SwiftUI/Combine re-work, which will address a lot of these concerns and I'd like to only address blocking comments for now to keep PCB progressing |
Yup, I'm fine with that. Just want to have some thoughts on the record for any future work we end up still doing in ReactiveSwift! |
📲 What
Part 1 in the breakdown of adding a new Confirm Details Screen
When a user selects an option in the rewards screen, navigate to the new screen.
🤔 Why
This screen will allow backers to confirm their pledge info before reaching the final checkout screen. This allows us to call PCP validation mutations before creating stipe intents and initializing the payment sheet.
🛠 How
This initiative is time-sensitive and is part of our more complicated and very important checkout flow. With that in mind, I'm choosing to follow existing patterns closely rather than spending more time refactoring or using new patterns like SwiftUI or Combine.
Navigation sources:
RewardAddOnSelectionViewControllerandRewardsCollectionViewController👀 See
✅ Acceptance criteria
⏰ TODO
Part 2 will be all about adding the pledge total section [MBL-1208] Part 2: Pledge Total Summary Section #1970