-
Notifications
You must be signed in to change notification settings - Fork 997
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
Update Afterpay logos for iOS users #4765
Conversation
…e-ios into joyceqin/RUN_MOBILESDK-3987
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.
This doesn't look quite right, why is it shifted half way down?
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 don't know why it shows up like that, but if you switch to viewing the source diff and back to rich diff it shows up correctly
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.
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 order to add the space before Cash App Afterpay but not Afterpay or Clearpay, we'd need to add a special case— do you think it's needed? Also, the info icon is there but not easily seen because we don't have a white asset for it. Do you know how to get 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.
IMO I think the Afterpay and Clearpay could use a space too.
To get the info icon to be white, you could try to do what is done in traitCollectionDidChange
and set the tint 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.
The info icon is a part of a NSAttributedString— I'm not sure how to access the info icon from traitCollectionDidChange
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.
Hmm, did you try?
You can do this by:
- Updating
attributedStringOfImageWithoutLink
to allow setting a tint color of the image attachment.
private func attributedStringOfImageWithoutLink(uiImage: UIImage, font: UIFont, tintColor: UIColor?) -> NSAttributedString {
let imageAttachment = NSTextAttachment()
imageAttachment.bounds = boundsOfImage(font: font, uiImage: uiImage)
if let tintColor {
imageAttachment.image = uiImage.withTintColor(tintColor, renderingMode: .alwaysTemplate)
} else {
imageAttachment.image = uiImage
}
return NSAttributedString(attachment: imageAttachment)
}
- Update how we build the
infoButton
inmakeAfterPayClearPayString()
:
let infoButton = attributedStringOfImageWithoutLink(uiImage: infoImage, font: titleFont, tintColor: theme.colors.parentBackground.contrastingColor)
- Update
traitCollectionDidChange
to recompute the attributed string:
override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
super.traitCollectionDidChange(previousTraitCollection)
afterpayMarkImageView.tintColor = theme.colors.parentBackground.contrastingColor
afterPayClearPayLabel.attributedText = makeAfterPayClearPayString()
}
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.
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.
Switching between the black and white logo seems fine, but there are some cases where this won't work! Imagine a case where a user is using the Appearance API to force their app into light mode (e.g., their app doesn't support dark mode). Then when the device is in dark mode, their app is in light mode. In this scenario, our info icon would be white and not visible.
Instead of using white for dark mode and black for light mode, we need to tint the image based on the color it is sitting on top of. The suggestion above does this, similar to how the afterpayMarkImageView
does this.
@@ -289,6 +289,7 @@ import Foundation | |||
return "multibanco" | |||
} | |||
} | |||
|
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.
This needed?
@@ -68,7 +68,7 @@ class PaymentSheetPaymentMethodTypeTest: XCTestCase { | |||
waitForExpectations(timeout: 10) | |||
// A Payment methods without a client-side asset... | |||
let loadExpectation = expectation(description: "Load form spec image") | |||
let image = PaymentSheet.PaymentMethodType.stripe(.amazonPay).makeImage { image in | |||
let image = PaymentSheet.PaymentMethodType.stripe(.amazonPay).makeImage(currency: nil) { image in |
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.
To avoid all these changes that aren't really needed, let's make currency default to nil in makeImage. That way you don't need to update many unrelated files. Once you add the default then you can come back to usages like this and revert the changes made.
case .wallet, .withPaymentMethod, .withPaymentDetails: | ||
return Image.link_logo.makeImage() | ||
} | ||
case .external(let paymentMethod, _): | ||
return PaymentSheet.PaymentMethodType.external(paymentMethod).makeImage( | ||
forDarkBackground: traitCollection?.isDarkMode ?? false, | ||
currency: 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.
Why aren't we passing in currency here and instead nil even though we have a currency
in scope?
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.
Currency isn't needed for external payment types, but I guess there's no harm in passing it anyways
} | ||
} | ||
|
||
var appearance: PaymentSheet.Appearance = PaymentSheet.Appearance.default { | ||
didSet { | ||
update() | ||
update(currency: currency) |
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.
Why do we need to pass currency into update, don't we have one in scope on self?
If it's okay for these strings to be unlocalized in master (e.g. this is for an unshipped feature), add the label New strings are localized on a weekly basis and are downloaded as part of the release process. For more details on how to localize a string, you can refer to this link. |
…e-ios into joyceqin/RUN_MOBILESDK-3987
Summary
Rebrand Afterpay in USD to be Cash App Afterpay, update shouldUseClearpayBrand to be determined by currency GBP
Motivation
https://jira.corp.stripe.com/browse/RUN_MOBILESDK-3987
Testing
Cash App Afterpay:

Regular Afterpay:

Clearpay Afterpay:

Changelog
[Changed] Updated Afterpay branding in the US to be Cash App Afterpay.