-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Display a “default” badge for the default PM #4333
Conversation
…s into joyceqin-MOBILESDK-2799
🚨 New dead code detected in this PR: SavedPaymentMethodCollectionView.swift:145 warning: Property 'needsVerticalPaddingForBadge' is assigned, but never used Please remove the dead code before merging. If this is intentional, you can bypass this check by adding the label ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with |
…from elements session logic
.../StripePaymentSheet/Source/Internal/API Bindings/v1-elements-sessions/ElementsCustomer.swift
Outdated
Show resolved
Hide resolved
.../StripePaymentSheet/Source/Internal/API Bindings/v1-elements-sessions/ElementsCustomer.swift
Outdated
Show resolved
Hide resolved
let defaultSavedPaymentMethod = paymentMethods.first { $0.stripeId == defaultPaymentMethod } | ||
return defaultSavedPaymentMethod | ||
} | ||
|
||
func getDefaultOrFirstPaymentMethod() -> STPPaymentMethod? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in PaymentSheetVerticalViewController, we have this logic for the initialSelection
if let customer = elementsSession.customer,
let defaultPaymentMethod = customer.getDefaultOrFirstPaymentMethod() {
customerDefault = CustomerPaymentOption.stripeId(defaultPaymentMethod.stripeId)
}
This seems to indicate that the first payment method can become the default. Is this true? Ultimately, i'm trying to wrap my head around when we would access defaultPaymentMethod vs defaultOrFirst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, the selected payment method should be the default if it exists or the first payment method in the customer's list of saved payment methods. However, the default badge itself should only be displayed if there is a set default in the elementsSession customer.
So we access defaultOrFirst when trying to determine the selected payment method, while we access defaultPaymentMethod to determine if the default badge should be shown and which payment method to show it on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for the explanation!
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
allowsPaymentMethodRemoval: configuration.paymentMethodRemove, | ||
allowsSetAsDefaultPM: false, | ||
showDefaultPMBadge: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing customerSheet changes will come at a later time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bella said that the CustomerSheet should not ever display a default badge because it's implied that the selected payment method in CustomerSheet is your default.
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
…s into joyceqin-MOBILESDK-2800
…ivate and deactivate, change isHidden to setHiddenIfNecessary
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
...heet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentOptionsViewController.swift
Outdated
Show resolved
Hide resolved
... Screen/Vertical Saved Payment Method Screen/VerticalSavedPaymentMethodsViewController.swift
Outdated
Show resolved
Hide resolved
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
@@ -69,6 +70,7 @@ class RowButton: UIView { | |||
imageView: UIImageView, | |||
text: String, | |||
subtext: String? = nil, | |||
showDefaultPMBadge: Bool = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing in "showDefaultPMBadge" feels like the wrong level of abstraction compared to the rest of the items being passed in.
We can remove the specifics of a "defaultPM" by passing in a more generic parameter called badgeText: String? = nil
, and renaming to badgeLabel
. Therefore, the init would look something like:
if let badgeText {
let badgeLabel = UILabel()
badgeLabel.font = appearance.scaledFont(for: appearance.font.base.regular, style: .caption1, maximumPointSize: 20)
badgeLabel.textColor = appearance.colors.textSecondary
badgeLabel.adjustsFontForContentSizeCategory = true
badgeLabel.text = badgeText
self.badgeLabel = badgeLabel
}
else {
self.badgeLabel = nil
}
Instance variable rename:
let badgeLabel: UILabel?
Then in SavedPaymentMethodRowButton.swift:
private lazy var rowButton: RowButton = {
return .makeForSavedPaymentMethod(paymentMethod: paymentMethod, appearance: appearance, badgeText: badgeText, rightAccessoryView: chevronButton, didTap: handleRowButtonTapped)
}()
private lazy var badgeText: String? = {
return showDefaultPMBadge ? String.Localized.default_text : nil
}()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just recently noticed that the Figma shows the badge with a different font weight when selected vs unselected, so I changed it to pass in defaultBadge as a UILabel? and adjust the weight when the state changes instead— does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a bit odd to pass in UILabel from a responsibilities perspective (e.g. If i have a view, feels like i should tell it what text to render, and it's up to that class to determine how to render it (might be a UILabel, might be a UITextView, etc..)
It seems that the selected state is available in the RowButton -- can't we hook into that to update the font weight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true— just updated to reflect that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SavedPaymentMethodRowButton.swift, can we add:
private lazy var rowButton: RowButton = {
return .makeForSavedPaymentMethod(paymentMethod: paymentMethod, appearance: appearance, badgeText: badgeText, rightAccessoryView: chevronButton, didTap: handleRowButtonTapped)
}()
private lazy var badgeText: String? = {
return showDefaultPMBadge ? String.Localized.default_text : nil
}()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
...Sheet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentMethodCollectionView.swift
Outdated
Show resolved
Hide resolved
📸 Snapshot TestNo snapshots generated
🛸 Powered by Emerge Tools |
…os into joyceqin-MOBILESDK-2800
Summary
Display a "default" badge for the default PM in vertical and horizontal mode across all UI shapes (payment sheet, flow controller, embedded)
Motivation
MOBILESDK-2800
Testing
Simulator.Screen.Recording.-.iPhone.16.-.2024-12-17.at.08.54.07.mp4
Simulator.Screen.Recording.-.iPhone.16.-.2024-12-17.at.08.54.31.mp4
Changelog