-
Notifications
You must be signed in to change notification settings - Fork 658
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
Add brand icons to card form #5069
Add brand icons to card form #5069
Conversation
Diffuse output:
APK
DEX
|
aac80fe
to
27f0cab
Compare
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.
One thing to check before approving.
How does this look with animations off? Curious if it doesn't animate, I wonder if this might cause tests to be flakey. Is this enabled by default otherwise I think you'll need to update the screenshot tests.
Does it look good with bigger font sizes? You can change that via this guy: https://github.com/stripe/stripe-android/blob/master/paymentsheet/src/main/java/com/stripe/android/paymentsheet/PaymentSheet.kt#L403
27f0cab
to
7d844aa
Compare
✅ The icons don't change size when the sizeScaleFactor changes. |
Could we update this so the trailing edge of the card brand icons lines up with the trailing edge of the cvc icon? |
@@ -45,6 +45,30 @@ enum class CardBrand( | |||
*/ | |||
private val variantMaxLength: Map<Pattern, Int> = emptyMap(), | |||
) { | |||
Visa( |
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 changed so that the order appears correctly in the UI? If so it might be better to have a getOrderedBrandList function. I can see adding a brand, and then not realizing I was impacting functionality, or changing the order around in the enum. The test would check it, but it might be better to be explicit. If there was a question you could look at the caller of getOrderedBrandList and see how it was being used.
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 one thing I don't like is that we will need to maintain this list if there is a new CardBrand
added, I think it can be easily missed.
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 this situation I feel like it's better to have an explicit list. Your test could also assert that the orderedBrandList function has the same length as CardBrand.values()
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 really see the difference between having a test that tests the order, and this. It will get caught either way.
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 agree that the test will catch the order, but if I don't understand that the order is used in a particular place in the code, I might just change the values in that function to make the test pass.
I think adding something to the test to make it clear, or modifying the function name to make it clear that there is a method to the ordering and it does matter and should not just be changed.
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 added a new field to CardBrand
called renderingOrder
let me know what you think of this approach
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.
sounds great, thanks James!
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt
Outdated
Show resolved
Hide resolved
This looks really slick! |
@csabol-stripe Done! |
a31aea7
to
54c9b81
Compare
54c9b81
to
95cbf84
Compare
Summary
Add brand icons to card form
Motivation
Card brand filtering
Testing
Screenshots
cardbrandicons.mov
Fixed icon alignment:
Changelog