Update cards in vertical mode #3595
Conversation
|
The following translations are missing in Lokalise. While they don't need to be downloaded into the codebase as part of this PR, they do need to exist in Lokalise so that they can be downloaded as part of the release process. Remove payment method By adding the label |
| /// - There is exactly one payment method available at init time. | ||
| /// - The single available payment method is not a co-branded card. | ||
| /// In this mode, the user can only delete the payment method; updating or selecting other payment methods is disabled. | ||
| private(set) lazy var isRemoveOnlyMode: Bool = { |
There was a problem hiding this comment.
It feels funny that this is lazy. It means that its value can change depending on when it's called, which doesn't seem right. Theoretically if paymentMethods changes before this is called, its value could be incorrect.
This seems like it should be calculated exactly at initialization time.
| /// - The single available payment method is not a co-branded card. | ||
| /// In this mode, the user can only delete the payment method; updating or selecting other payment methods is disabled. | ||
| private(set) lazy var isRemoveOnlyMode: Bool = { | ||
| let hasCoBrandedCards = !paymentMethodRows.filter { $0.paymentMethod.isCoBrandedCard }.isEmpty |
There was a problem hiding this comment.
why not paymentMethods.filter { $0.isCoBrandedCard }...
| @@ -165,6 +190,8 @@ class VerticalSavedPaymentMethodsViewController: UIViewController { | |||
| } | |||
|
|
|||
| private func completeSelection() { | |||
| // Edge-case: Dismiss `UpdateViewController` if presented, this can occur if `completeSelection` is called before `UpdateViewController` is popped when we remove the last payment method via the `UpdateViewController` | |||
There was a problem hiding this comment.
Nice comment. Can we eliminate this edge case?
Telling the bottomSheetController to pop the content VC multiple times doesn't seem right, since we could inadvertently pop multiple VCs off when we only mean to pop the update VC. Maybe we should either tell the bottom sheet controller to pop the updateViewController specifically (and have that no-op if it doesn't find it) or otherwise make sure we're only popping the content VC once.
There was a problem hiding this comment.
Yeah this is a little confusing. Today, when this edge-case occurs and we double call popContentViewController it will actually no-op on the second call since the bottomSheetController on the update VC is nil after it has been popped since it is no longer in the bottom sheet content stack, but this definitely isn't obvious. From what I can tell popping a VC multiple times is safe due to this logic.
There was a problem hiding this comment.
Ah ok. I feel like there's still an even better way to write this so that like, the update delegate's remove method looks like:
- Remove the PM
- Pop the VC
- Exit the flow if necessary.
but I won't block on it.
| guard let oldButton = paymentMethodRows.first(where: { $0.paymentMethod.stripeId == paymentMethod.stripeId }), | ||
| let oldButtonModelIndex = paymentMethodRows.firstIndex(of: oldButton), | ||
| let oldButtonViewIndex = stackView.arrangedSubviews.firstIndex(of: oldButton) else { | ||
| return |
There was a problem hiding this comment.
Should probably stpAssert here
| newButton.state = .editing(allowsRemoval: canRemovePaymentMethods, | ||
| allowsUpdating: updatedPaymentMethod.isCoBrandedCard && isCBCEligible) |
There was a problem hiding this comment.
I wonder if this could be newButton.state = oldButton.state?
There was a problem hiding this comment.
Ah yep. I was thinking canRemovePaymentMethods could change after an update but it definitely won't after a second look.
| @@ -24,13 +24,18 @@ final class VerticalSavedPaymentMethodsViewControllerSnapshotTests: STPSnapshotT | |||
| _test_VerticalSavedPaymentMethodsViewControllerSnapshotTests(darkMode: false, appearance: ._testMSPaintTheme) | |||
| } | |||
|
|
|||
| func _test_VerticalSavedPaymentMethodsViewControllerSnapshotTests(darkMode: Bool, appearance: PaymentSheet.Appearance = .default) { | |||
| func test_VerticalSavedPaymentMethodsViewControllerSnapshotTestsLightModeRemoveOnlyMode() { | |||
There was a problem hiding this comment.
super nit: I'd remove lightMode in this test method name.
Summary
UpdateCardViewControllerDelegateto pass itself through the delegate methods than update it so the users pop the update VC rather than theUpdateCardViewControllerpopping itself.isCBCEligibleand implementsUpdateCardViewControllerDelegateinVerticalSavedPaymentMethodsViewControllerUIStackViewsetup inUpdateCardViewControllerto resolve a janky push animation (same as we saw earlier in the project)UpdateCardViewControllerSnapshotTeststo not define a hardcoded height for it's view while testingVerticalSavedPaymentMethodsViewControllerto only allow removal and not selection or updating when 1 payment method is present and it is not update-able. See below for figmaMotivation
Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-05-24.at.09.06.57.mp4
Testing
Changelog
N/A