Skip to content
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

[MBL-1210] Post campaign checkout #1984

Merged
merged 13 commits into from Mar 20, 2024
Merged

[MBL-1210] Post campaign checkout #1984

merged 13 commits into from Mar 20, 2024

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Mar 19, 2024

📲 What

Add a mostly completed post campaign checkout page. This page is presented after the confirm details page. See TODO section for remaining work.

Note: This (including the TODOs) only update the UI. The payment sheet is not wired up yet at all.

👀 See

Jira

image

♿️ Accessibility

  • Tap targets use minimum of 44x44 pts dimensions
  • Dynamic Type should be fine since this isn't adding any new components

Note: VoiceOver will be tested later to unblock work.

✅ Acceptance criteria

  • Checkout page shows up (for rewards that include shipping) and matches the mocks with the exceptions listed below

⏰ TODO

TODO in follow-up prs to unblock work

  • Update rewards summary table to fully support the checkout page. This requires supporting nil shipping and figuring out why bonus support hides on this page
  • Update CTA to a) show "charged immediately" warning, probably by creating a postCampaignPledge context and b) properly set fields & respond to login flow
  • Add snapshot tests once the above UI issues are fixed

@ifosli ifosli self-assigned this Mar 19, 2024
@@ -44,6 +44,8 @@ final class PostCampaignPledgeRewardsSummaryViewController: UIViewController {
override func viewDidLoad() {
super.viewDidLoad()

self.view.isHidden = true
self.pledgeTotalViewController.view.isHidden = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It's important to never put anything in bindStyles that can be overwritten later. Anything to do with initial state needs to go in viewDidLoad instead. I spent hours trying to debug why the table wasn't showing up on the checkout screen 😭

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually causes a slight issue where a small piece of this view is shown while the table data loads. This isn't as smooth of a UX IMO but I'm not sure what the fix is.
Simulator Screen Recording - iPhone 15 Pro - 2024-03-19 at 14 02 56

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling that out! For some reason, the separator view was still showing... I just hid that as well and the table is back to being fully hidden before data loads!

import Library
import UIKit

struct PledgeViewControllerHelpers {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is literally copy-pasted from the pledgeViewController with no changes; no need to review carefully.

@nativeksr
Copy link
Collaborator

nativeksr commented Mar 19, 2024

1 Warning
⚠️ Big PR

SwiftFormat found issues:

File Rules
Kickstarter-iOS/Features/PledgePaymentMethods/Controller/PostCampaignPledgeRewardsSummaryViewController.swift:49:1 warning: (redundantSelf) Insert/remove explicit self where applicable.
Kickstarter-iOS/Features/PledgePaymentMethods/Controller/PostCampaignPledgeRewardsSummaryViewController.swift:168:1 warning: (indent) Indent code in accordance with the scope level.
Kickstarter-iOS/Features/PledgePaymentMethods/Controller/PostCampaignPledgeRewardsSummaryViewController.swift:169:1 warning: (blankLinesAroundMark) Insert blank line before and after MARK: comments.

Generated by 🚫 Danger

private lazy var pledgeCTAContainerView: PledgeViewCTAContainerView = {
PledgeViewCTAContainerView(frame: .zero)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
// TODO: Add self as delegate and add support for delegate methods.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if we have a ticket for wiring up the payment flow here; let me know if we do and I'll link it

Copy link
Contributor Author

@ifosli ifosli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The initial prs are neatly organized if you want to review them in order. However, that merge conflict was evil, so I gave up on organized commits after that.

@ifosli ifosli marked this pull request as ready for review March 19, 2024 18:35
@ifosli ifosli requested a review from scottkicks March 19, 2024 18:36
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first piece is looking good!
We just need the bonus support amount to also be shown in the pledge rewards summary table. It looks like the bonus amount is being passed along to the table correctly. I'm not seeing why the table isn't displaying the bonus item row though.

@@ -44,6 +44,8 @@ final class PostCampaignPledgeRewardsSummaryViewController: UIViewController {
override func viewDidLoad() {
super.viewDidLoad()

self.view.isHidden = true
self.pledgeTotalViewController.view.isHidden = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually causes a slight issue where a small piece of this view is shown while the table data loads. This isn't as smooth of a UX IMO but I'm not sure what the fix is.
Simulator Screen Recording - iPhone 15 Pro - 2024-03-19 at 14 02 56

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG one the circleci issues are resolved

@ifosli ifosli merged commit 09df982 into main Mar 20, 2024
5 checks passed
@ifosli ifosli deleted the postCampaignCheckout branch March 20, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants