-
Notifications
You must be signed in to change notification settings - Fork 12
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
Convergence asset update #282
Conversation
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.
Couple of thoughts:
-
If we go with this approach to silently swap views then we will need to communicate these changes to avoid merchant confusion
-
This changes public API so it will be a breaking change. Have we considered a non-breaking approach and deprecating then removing no longer supported views?
public enum ColorPalette { | ||
case blackOnMint | ||
case mintOnBlack | ||
case whiteOnBlack | ||
case blackOnWhite |
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.
Is this a breaking change?
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.
yes but that was intentional to get the US merchants to update the UI since it is new. Also we're not marketing non-US partners to update but we should have good documentation on how the new values map to the old ones just in case they do end up updating for some reason
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.
Every other asset is a PNG, is there a reason we are moving to SVG?
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.
SVG is better for static images like logos since it scales better, but tbh I didn't think it would make a difference and wanted to use the same assets as Android, was there a reason why PNG or PDF was used before?
foregroundImageView.tintColor = color.foreground | ||
} | ||
|
||
private func resolveColorScheme(palette: ColorPalette?) -> (foreground: UIColor?, background: UIColor?) { |
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.
Does palette
need to be optional here?
if Afterpay.isCashAppAfterpayRegion { | ||
var color = colorScheme.lightPalette.foregroundCashAppAfterpay | ||
if traitCollection.userInterfaceStyle == .dark { | ||
color = colorScheme.darkPalette.foregroundCashAppAfterpay | ||
} | ||
return color | ||
} |
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.
Maybe this can be cleaned up with a guard?
Sources/Afterpay/Afterpay.swift
Outdated
@@ -416,6 +416,10 @@ internal var brand: Brand { | |||
Brand.forLocale(locale: getLocale()) | |||
} | |||
|
|||
public var isCashAppAfterpayRegion: Bool { | |||
Brand.forLocale(locale: getLocale()).isCashAppAfterpayRegion |
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 not completely sure but you might want to double check that getLocale()
works with getV3Configuration
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's ok? it looks like the v3 endpoint ends up calling setConfiguration which is required to have a locale.
Also that same exact code is used in several other places:
https://github.com/afterpay/sdk-ios/blob/master/Sources/Afterpay/Views/LogoView.swift#L68
https://github.com/afterpay/sdk-ios/blob/master/Sources/Afterpay/Afterpay.swift#L416
I've updated to just use the brand
computed property
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 this is ok here - correct me if I'm wrong @mmroz, but my understanding is that for any of the assets to work anyway the Brand needs to be set which is done by having a locale set through via setConfiguration
. Configuration
also contains things like min
/max
amounts for SDK consumers so that they can show the Pay with ${brand}
messaging.
So I believe if consumers of the SDK want to use v3 + the assets, they need to set Configuration
and CheckoutV3Configuration
.
59030fa
to
7663874
Compare
7663874
to
dfe6f3b
Compare
|
@@ -70,7 +70,7 @@ final class CartViewController: UIViewController, UITableViewDataSource { | |||
|
|||
if Afterpay.enabled { | |||
let payButton: UIButton = PaymentButton( | |||
colorScheme: .dynamic(lightPalette: .blackOnMint, darkPalette: .mintOnBlack), | |||
colorScheme: .dynamic(lightPalette: .default, darkPalette: .lightMono), |
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.
did you mean to change the colors here? i thought the updated mappings were:
.blackOnMint
→ .alt
.mintOnBlack
→ .default
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.
yea that's true, I'm actually going to overhaul this entire view to include more permutations and ability to select region
@@ -12,7 +12,7 @@ private let localeLanguages: [Locale: Drawables] = [ | |||
Locales.enAU: .enAfterpay, | |||
Locales.enGB: .enClearpay, | |||
Locales.enNZ: .enAfterpay, | |||
Locales.enUS: .enAfterpay, | |||
Locales.enUS: .enCashAppAfterpay, | |||
Locales.enUSposix: .enAfterpay, |
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.
should Locales.enUSposix
be changed to .enCashAppAfterpay
too, since it's now a Cash App Afterpay locale according to Brand.swift?
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.
Yes, thank you for the catch!
override public init(colorScheme: ColorScheme = .static(.blackOnMint)) { | ||
override public init(colorScheme: ColorScheme = .static(.default)) { | ||
if Afterpay.isCashAppAfterpayRegion { | ||
assertionFailure("Cash App Afterpay badge is not supported") |
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.
what's the context behind not supporting BadgeView and CompactBadgeView? what happens if a merchant was previously using them in a Cash App Afterpay region?
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.
It would crash, which is what we would want since there are no assets associated with them in the CashAppAfterpay branding. We should include in the documentation to use the Lockup assets
Summary of Changes
isCashAppAfterpayRegion
https://docs.google.com/document/d/1SZYkcTyi5Y5NyclJJu6wHWEZAT6oHFyGZVgX5MhhTI8/edit?tab=t.0
Items of Note
Submission Checklist