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-1607] Add Estimated Shipping View to Late Pledge Checkout #2133

Merged
merged 15 commits into from
Aug 28, 2024

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Aug 26, 2024

📲 What

Adds a new SwiftUI to display estimated shipping cost

🤔 Why

JIRA TICKET

🛠 How

  • Adds the new SwiftUI View EstimatedShippingCheckoutView to PostCampaignCheckoutViewController
  • Add snapshot tests

👀 See

thisone

✅ Acceptance criteria

  • New view matches the new designs
  • If there is no estimated shipping or if shipping is not enabled for the selected reward, we don't show this new view.

@scottkicks scottkicks force-pushed the scott/mbl-1607/update-late-pledge-ui branch from b6d0c82 to 945f18e Compare August 26, 2024 20:19
@kickstarter kickstarter deleted a comment from nativeksr Aug 27, 2024
@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@scottkicks scottkicks marked this pull request as ready for review August 27, 2024 15:42
@scottkicks scottkicks requested a review from ifosli August 27, 2024 19:49
Copy link
Contributor

@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.

I'm concerned about how this is making the late pledge VC layout worse, even when there's no estimated shipping. Can you confirm that Alison is okay with launching with this layout (extra left and right margins for the pledge view and weird gap between the scroll view and the pledge cta) even for late pledges without estimated shipping? If not, I would strongly recommend fixing it in this pr but I'm also okay with filing a follow-up ticket/fixing it in a separate pr (follow-up ticket should probably happen anyways so we can fix it for the next release, if it ends up not being launch blocking).

Comment on lines +111 to +114
private lazy var estimatedShippingStackView: UIStackView = {
UIStackView(frame: .zero)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ifosli For the correct margins to be applied, the estimated shipping view needs to be added to a new stack view as a subview. This is true for UIKit views as well. I spent time rebuilding the view with UIKit to verify this.

@scottkicks scottkicks merged commit 4decc8f into main Aug 28, 2024
5 checks passed
@scottkicks scottkicks deleted the scott/mbl-1607/update-late-pledge-ui branch August 28, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants