Show payment methods in vertical saved list #3573

porter-stripe · 2024-05-09T18:37:22Z

Summary

Adds ability to display and select payment methods in the VerticalSavedPaymentOptionsViewController
Adds delegates to communicate between the new PaymentMethodRowButton <> VerticalSavedPaymentOptionsViewController <> PaymentSheetVerticalViewController
Still grappling with the janky push animation, will fix later.

Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-05-09.at.14.50.26.mp4

Motivation

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

Testing

Manual
Snapshots

Changelog

N/A

porter-stripe · 2024-05-09T18:53:18Z

...t/Saved Payment Method Screen/Vertical Saved Payment Method Screen/CheckmarkCircleView.swift

+import UIKit
+
+/// Draws a circle with the desired fill color with a white checkmark in the center
+final class CheckmarkCircleView: UIView {


Omitted snapshot tests for this class and relying on the VerticalSavedPaymentOptionsViewControllerSnapshotTests. Can add them if we feel they will be useful.

👍 I don't think this needs its own tests

porter-stripe · 2024-05-09T18:53:44Z

...aved Payment Method Screen/Vertical Saved Payment Method Screen/PaymentMethodRowButton.swift

+    // TODO(porter) Add did delete and did update
+}
+
+final class PaymentMethodRowButton: UIView {


Same, omitted snapshot tests for this view and relied on VerticalSavedPaymentOptionsViewControllerSnapshotTests.

porter-stripe · 2024-05-09T18:56:32Z

... Screen/Vertical Saved Payment Method Screen/VerticalSavedPaymentOptionsViewController.swift

        return label
    }()
+
+    private lazy var paymentMethodRows: [(paymentMethod: STPPaymentMethod, button: PaymentMethodRowButton)] = {


Not sure how I feel about this. It's tricky to associate a STPPaymentMethod with a PaymentMethodRowButton when:

Not using a table/collection view

Not muddying up the PaymentMethodRowButton.ViewModel with an STPPaymentMethod.

Open to suggestions

I don't have strong ideas either. Why not have the payment method as a property of the view (either in the viewmodel or just directly)?

I just felt like the PaymentMethodRowButton and PaymentMethodRowButton.ViewModel shouldn't know about an entire STPPaymentMethod and it should just know the data it needs to draw itself, no other reason.

porter-stripe · 2024-05-09T22:00:14Z

...rce/PaymentSheet/Saved Payment Method Screen/VerticalSavedPaymentOptionsViewController.swift

@@ -1,91 +0,0 @@
-//
-//  VerticalSavedPaymentOptionsViewController.swift


ooof, git doesn't recognize it was moved :(

yuki-stripe

Awesome!

... Screen/Vertical Saved Payment Method Screen/VerticalSavedPaymentOptionsViewController.swift

yuki-stripe · 2024-05-09T22:00:05Z

...aved Payment Method Screen/Vertical Saved Payment Method Screen/PaymentMethodRowButton.swift

+    struct ViewModel {
+        let appearance: PaymentSheet.Appearance
+        let text: String
+        let image: UIImage
+        // TODO(porter) Add can remove and can update
+    }


what's the idea behind putting this in a struct ViewModel rather than just making these properties on the PaymentMethodRowButton class directly?

Probably not totally needed right now, but I anticipate this will scale better and am hoping in the future to create view models for PaymentMethodRowButton from a STPPaymentMethod. Mainly trying to keep the size of PaymentMethodRowButton.init down and offload the injection building somewhere else. Not totally needed but kinda like the direction.

extension STPPaymentMethod { var rowButtonViewModel: PaymentMethodRowButton.ViewModel {...} } // Somewhere else let rowButtons = savedPaymentMethods.map{PaymentMethodRowButton(appearance: appearance, viewModel: $0.rowButtonViewModel)}

yuki-stripe · 2024-05-09T22:42:33Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentOption+Images.swift

+    /// Returns an image to display inside a row representing the given payment option in the saved PM row view
+    func makeSavedPaymentMethodRowImage() -> UIImage {


Why not reuse makeSavedPaymentMethodCellImage?

makeSavedPaymentMethodCellImage uses carousel images for card brands and for SEPA. We need the images that are used in the PAN field for card brand rather than the carousel images. Here the diff if we use makeSavedPaymentMethodCellImage, notice visa & SEPA look a little different than what is laid out in the mocks. The differences would be more sark in dark mode but I don't think we have mocks for dark mode.

Mock

Ohh right, we have 'carousel' versions of these. I guess we can delete those once we finish LPM visibility and the horizontal saved pm carousel goes away.

yuki-stripe · 2024-05-09T22:43:27Z

...t/Saved Payment Method Screen/Vertical Saved Payment Method Screen/CheckmarkCircleView.swift

+import UIKit
+
+/// Draws a circle with the desired fill color with a white checkmark in the center
+final class CheckmarkCircleView: UIView {


👍 I don't think this needs its own tests

...aved Payment Method Screen/Vertical Saved Payment Method Screen/PaymentMethodRowButton.swift

yuki-stripe · 2024-05-09T23:12:16Z

...aved Payment Method Screen/Vertical Saved Payment Method Screen/PaymentMethodRowButton.swift

+            shadowRoundedRect.leadingAnchor.constraint(equalTo: leadingAnchor),
+            shadowRoundedRect.trailingAnchor.constraint(equalTo: trailingAnchor),
+            shadowRoundedRect.bottomAnchor.constraint(equalTo: bottomAnchor),
+            shadowRoundedRect.heightAnchor.constraint(equalToConstant: height),


Why is this needed?

Without explicitly setting the height of the shadowRect it is just sized the minimum to fit the content. Is there a better way to do this?

Removing the shadowRoundedRect.heightAnchor.constraint(equalToConstant: height):

Looks like the stack view doesn't have top or bottom margins.

stackView.directionalLayoutMargins = PaymentSheetUI.defaultMargins

Oh good catch, ty!

yuki-stripe · 2024-05-09T23:15:53Z

... Screen/Vertical Saved Payment Method Screen/VerticalSavedPaymentOptionsViewController.swift

        return label
    }()
+
+    private lazy var paymentMethodRows: [(paymentMethod: STPPaymentMethod, button: PaymentMethodRowButton)] = {


I don't have strong ideas either. Why not have the payment method as a property of the view (either in the viewmodel or just directly)?

yuki-stripe · 2024-05-09T23:20:38Z

...ePaymentSheetTests/PaymentSheet/VerticalSavedPaymentOptionsViewControllerSnapshotTests.swift

+        return [STPFixtures.paymentMethod(),
+                STPFixtures.usBankAccountPaymentMethod(),
+                STPFixtures.usBankAccountPaymentMethod(bankName: "BANK OF AMERICA"),
+                STPFixtures.usBankAccountPaymentMethod(bankName: "STRIPE"),
+                STPFixtures.sepaDebitPaymentMethod(), ]


We have two ways to provide fixtures unfortunately. THe old Obj-C way is static methods on STPFixtures class. THe new way is to provide extensions on the thing. See STPFixtures+PaymentSheet.swift. It already has STPPaymentMethod._testSEPA and I think it's a more ergonomic and scalable way we should use going forward.

Should probably make a ticket to migrate us off of STPFixtures

Filed here!. I'll leave as-is for now but I'll pick this up next time I'm on run.

porter-stripe added 8 commits May 9, 2024 08:45

Start showing payment methods in a StackView

be5deff

Add snapshot tests, allow selection

d0e5edd

Add snapshot tests for bank accts and SEPA debit

c70c42f

Lint

5a43ed5

Little bit of clean up

a61229c

Share latest

4691297

Notify PaymentSheetVerticalViewController

19dd9e5

Improve UX when selecting and lint

3c5d206

porter-stripe changed the title ~~Porter/mobilesdk 2024~~ Show payment methods in vertical saved list May 9, 2024

stripe deleted a comment from github-actions bot May 9, 2024

Don't make extension public

131d669

porter-stripe commented May 9, 2024

View reviewed changes

porter-stripe added 5 commits May 9, 2024 13:10

Rename protocol

7d3c03c

Delete TODO comment

c8a5810

Merge branch 'master' into porter/MOBILESDK-2024

35206f1

Update snapshots for new logo

c2a6da0

Update spacing per figma

d20b668

porter-stripe marked this pull request as ready for review May 9, 2024 20:51

porter-stripe requested review from a team as code owners May 9, 2024 20:51

Only set selected if previously was selected

246d877

porter-stripe requested a review from yuki-stripe May 9, 2024 20:53

porter-stripe added 2 commits May 9, 2024 15:07

Handle double tap edge case

553d453

Lint

946a613

porter-stripe commented May 9, 2024

View reviewed changes

yuki-stripe reviewed May 9, 2024

View reviewed changes

porter-stripe added 2 commits May 9, 2024 17:51

Rename to VerticalSavedPaymentMethodsViewController

4b08cc3

Use addAndPinSubview where possible

ee46679

porter-stripe added 4 commits May 9, 2024 23:05

Fix snapshots

1b64726

Add top and bottom margins to stackview

67dbc25

Delete dead code and add comments

2fd92cc

lint

a99ccea

porter-stripe requested a review from yuki-stripe May 10, 2024 15:53

yuki-stripe approved these changes May 10, 2024

View reviewed changes

porter-stripe merged commit a5b1726 into master May 10, 2024
4 checks passed

porter-stripe deleted the porter/MOBILESDK-2024 branch May 10, 2024 16:11

yuki-stripe mentioned this pull request May 14, 2024

Add ability to toggle edit mode for vertical mode #3576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show payment methods in vertical saved list #3573

Show payment methods in vertical saved list #3573

porter-stripe commented May 9, 2024 •

edited

porter-stripe May 9, 2024

yuki-stripe May 9, 2024

porter-stripe May 9, 2024

porter-stripe May 9, 2024 •

edited

yuki-stripe May 9, 2024

porter-stripe May 10, 2024

porter-stripe May 9, 2024

yuki-stripe left a comment

yuki-stripe May 9, 2024

porter-stripe May 10, 2024 •

edited

yuki-stripe May 9, 2024

porter-stripe May 10, 2024

yuki-stripe May 10, 2024

yuki-stripe May 9, 2024

yuki-stripe May 9, 2024

porter-stripe May 10, 2024

yuki-stripe May 10, 2024

porter-stripe May 10, 2024

yuki-stripe May 9, 2024

yuki-stripe May 9, 2024

yuki-stripe May 9, 2024

porter-stripe May 10, 2024

		@@ -1,91 +0,0 @@
		//
		// VerticalSavedPaymentOptionsViewController.swift

		/// Returns an image to display inside a row representing the given payment option in the saved PM row view
		func makeSavedPaymentMethodRowImage() -> UIImage {

Show payment methods in vertical saved list #3573

Show payment methods in vertical saved list #3573

Conversation

porter-stripe commented May 9, 2024 • edited

Summary

Motivation

Testing

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porter-stripe May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuki-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porter-stripe May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mock

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porter-stripe commented May 9, 2024 •

edited

porter-stripe May 9, 2024 •

edited

porter-stripe May 10, 2024 •

edited