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

Embedded update pt 2 - update view #4150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yuki-stripe
Copy link
Collaborator

Summary

Previous PR: #4141

Makes EmbeddedPaymentElement view a view that contains embedded view so we can swap to the updated embedded view with an animation.

Simulator.Screen.Recording.-.snapshot.tester.iPhone.12.mini.16.4.-.2024-10-16.at.11.40.47.mp4

Still to come:

  • Restore previous customer input + E2E tests (e.g. load PI -> fill out card form -> update to SI -> expect form to be preserved but w/o checkbox)
  • Make confirm handle in-flight and failed update calls.
  • (Bonus) Cancel network calls etc. from previous update to reduce battery/network usage. Can apply this to FC.update as well.

Motivation

https://jira.corp.stripe.com/browse/MOBILESDK-2583

Testing

See snapshot test.

Changelog

Not user facing

Copy link
Collaborator

@porter-stripe porter-stripe left a comment

Choose a reason for hiding this comment

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

Few non blocking Q's, nice!

embeddedPaymentMethodsView: embeddedPaymentMethodsView
)

self.containerView.updateSuperviewHeight = { [weak self] in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to not just remove containerView and just return it directly on view? Guessing it's to avoid casting between UIView and the container view?

/// The view that's vended to the merchant, containing the embedded view. We use this to be able to swap out the embedded view with an animation when `update` is called.
class EmbeddedPaymentElementContainerView: UIView {
var updateSuperviewHeight: () -> Void = {}
var view: UIView
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason these aren't private (excluding updateSuperviewHeight, although that feels like it should come in at init time and be optional)? Also wondering it it ever makes sense to use this class when view is not of type EmbeddedPaymentMethodsView. Should it just be EmbeddedPaymentMethodsView over UIView in this class?

@_spi(STP) @testable import StripeUICore
import XCTest

class EmbeddedPaymentElementSnapshotTests: STPSnapshotTestCase, EmbeddedPaymentElementDelegate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we planning to add more tests to this file? If so do we have a ticket or will that happen as a follow up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is all the transparent padding expected? Would that be the merchant's UI/background in the real world?

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.

2 participants