Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add brand icons to card form #5069

Merged
merged 3 commits into from May 31, 2022
Merged

Add brand icons to card form #5069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73d97ec
Add brand icons to card form
jameswoo-stripe May 25, 2022
1e2a029
Update RowElementUI and screenshot tests
jameswoo-stripe May 27, 2022
95cbf84
Refactor ordering
jameswoo-stripe May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Expand Up @@ -3,7 +3,8 @@

### PaymentSheet
* [DEPRECATED][5061](https://github.com/stripe/stripe-android/pull/5061) Add Deprecated annotation to old primaryButtonColor api.
* [FIXED][5068](https://github.com/stripe/stripe-android/pull/5068) Fix missing theming for add lpm button and notes text
* [FIXED][5068](https://github.com/stripe/stripe-android/pull/5068) Fix missing theming for add lpm button and notes text.
* [ADDED][5069](https://github.com/stripe/stripe-android/pull/5069) Add card brand icons to card details form.

### Payments
* [FIXED][5079](https://github.com/stripe/stripe-android/pull/5079) Add 3ds2 url to list of completion URLs so callbacks work correctly.
Expand Down
18 changes: 18 additions & 0 deletions payments-core/src/test/java/com/stripe/android/model/CardBrandTest.kt
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ package com.stripe.android.model
import com.google.common.truth.Truth.assertThat
import com.stripe.android.CardNumberFixtures
import kotlin.test.Test
import kotlin.test.assertContentEquals
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertTrue
Expand Down Expand Up @@ -176,4 +177,21 @@ class CardBrandTest {
assertThat(CardBrand.fromCardNumber("561243"))
.isEqualTo(CardBrand.MasterCard)
}

@Test
fun cardBrandIsOrdered() {
// Ordered for rendering purposes in the card field
assertContentEquals(
arrayOf(
CardBrand.Visa,
CardBrand.MasterCard,
CardBrand.AmericanExpress,
CardBrand.Discover,
CardBrand.JCB,
CardBrand.DinersClub,
CardBrand.UnionPay
),
CardBrand.orderedBrands.toTypedArray()
)
}
}
1 change: 1 addition & 0 deletions payments-model/api/payments-model.api
View file
Open in desktop
Expand Up @@ -153,6 +153,7 @@ public final class com/stripe/android/model/CardBrand : java/lang/Enum {

public final class com/stripe/android/model/CardBrand$Companion {
public final fun fromCode (Ljava/lang/String;)Lcom/stripe/android/model/CardBrand;
public final fun getOrderedBrands ()Ljava/util/List;
}

public final class com/stripe/android/model/CardFunding : java/lang/Enum {
Expand Down
76 changes: 47 additions & 29 deletions payments-model/src/main/java/com/stripe/android/model/CardBrand.kt
View file
Open in desktop
Expand Up @@ -44,7 +44,38 @@ enum class CardBrand(
* By default, a [CardBrand] does not have variants.
*/
private val variantMaxLength: Map<Pattern, Int> = emptyMap(),

/**
* The rendering order in the card details cell
*/
private val renderingOrder: Int
) {
Visa(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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()

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds great, thanks James!

"visa",
"Visa",
R.drawable.stripe_ic_visa,
pattern = Pattern.compile("^(4)[0-9]*$"),
partialPatterns = mapOf(
1 to Pattern.compile("^4$")
),
renderingOrder = 1
),

MasterCard(
"mastercard",
"Mastercard",
R.drawable.stripe_ic_mastercard,
pattern = Pattern.compile(
"^(2221|2222|2223|2224|2225|2226|2227|2228|2229|222|223|224|225|226|" +
"227|228|229|23|24|25|26|270|271|2720|50|51|52|53|54|55|56|57|58|59|67)[0-9]*$"
),
partialPatterns = mapOf(
1 to Pattern.compile("^2|5|6$"),
2 to Pattern.compile("^(22|23|24|25|26|27|50|51|52|53|54|55|56|57|58|59|67)$")
),
renderingOrder = 2
),

AmericanExpress(
"amex",
"American Express",
Expand All @@ -55,7 +86,8 @@ enum class CardBrand(
pattern = Pattern.compile("^(34|37)[0-9]*$"),
partialPatterns = mapOf(
1 to Pattern.compile("^3$")
)
),
renderingOrder = 3
),

Discover(
Expand All @@ -66,6 +98,7 @@ enum class CardBrand(
partialPatterns = mapOf(
1 to Pattern.compile("^6$")
),
renderingOrder = 4
),

/**
Expand All @@ -82,7 +115,8 @@ enum class CardBrand(
1 to Pattern.compile("^3$"),
2 to Pattern.compile("^(35)$"),
3 to Pattern.compile("^(35[2-8])$")
)
),
renderingOrder = 5
),

/**
Expand All @@ -102,31 +136,8 @@ enum class CardBrand(
),
variantMaxLength = mapOf(
Pattern.compile("^(36)[0-9]*$") to 14
)
),

Visa(
"visa",
"Visa",
R.drawable.stripe_ic_visa,
pattern = Pattern.compile("^(4)[0-9]*$"),
partialPatterns = mapOf(
1 to Pattern.compile("^4$")
),
),

MasterCard(
"mastercard",
"Mastercard",
R.drawable.stripe_ic_mastercard,
pattern = Pattern.compile(
"^(2221|2222|2223|2224|2225|2226|2227|2228|2229|222|223|224|225|226|" +
"227|228|229|23|24|25|26|270|271|2720|50|51|52|53|54|55|56|57|58|59|67)[0-9]*$"
),
partialPatterns = mapOf(
1 to Pattern.compile("^2|5|6$"),
2 to Pattern.compile("^(22|23|24|25|26|27|50|51|52|53|54|55|56|57|58|59|67)$")
)
renderingOrder = 6
),

UnionPay(
Expand All @@ -136,15 +147,17 @@ enum class CardBrand(
pattern = Pattern.compile("^(62|81)[0-9]*$"),
partialPatterns = mapOf(
1 to Pattern.compile("^6|8$"),
)
),
renderingOrder = 7
),

Unknown(
"unknown",
"Unknown",
R.drawable.stripe_ic_unknown,
cvcLength = setOf(3, 4),
partialPatterns = emptyMap()
partialPatterns = emptyMap(),
renderingOrder = -1
);

val maxCvcLength: Int
Expand Down Expand Up @@ -215,7 +228,7 @@ enum class CardBrand(
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
fun getCardBrands(cardNumber: String?): List<CardBrand> {
if (cardNumber.isNullOrBlank()) {
return listOf(Unknown)
return orderedBrands
}

return getMatchingCards(cardNumber).takeIf {
Expand All @@ -236,6 +249,11 @@ enum class CardBrand(
return values().firstOrNull { it.code.equals(code, ignoreCase = true) } ?: Unknown
}

val orderedBrands = values()
.toList()
.filter { it.renderingOrder > 0 }
.sortedBy { it.renderingOrder }

private const val CVC_COMMON_LENGTH: Int = 3
}
}
1 change: 1 addition & 0 deletions payments-ui-core/api/payments-ui-core.api
View file
Open in desktop
Expand Up @@ -532,6 +532,7 @@ public final class com/stripe/android/ui/core/elements/StaticTextElementUIKt {
}

public final class com/stripe/android/ui/core/elements/TextFieldUIKt {
public static final fun AnimatedIcons (Ljava/util/List;ZLandroidx/compose/runtime/Composer;I)V
public static final fun TextField-PwfN4xk (Lcom/stripe/android/ui/core/elements/TextFieldController;Landroidx/compose/ui/Modifier;IZLkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)V
public static final fun TextFieldSection-VyDzSTg (Lcom/stripe/android/ui/core/elements/TextFieldController;Landroidx/compose/ui/Modifier;Ljava/lang/Integer;IZLkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)V
}
Expand Down
17 changes: 13 additions & 4 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt
View file
Open in desktop
Expand Up @@ -65,11 +65,20 @@ internal class CardNumberController constructor(
override val trailingIcon: Flow<TextFieldIcon?> = _fieldValue.map {
val cardBrands = CardBrand.getCardBrands(it)
if (accountRangeService.accountRange != null) {
TextFieldIcon(accountRangeService.accountRange!!.brand.icon, isIcon = false)
} else if (cardBrands.size == 1) {
TextFieldIcon(cardBrands.first().icon, isIcon = false)
TextFieldIcon.Trailing(accountRangeService.accountRange!!.brand.icon, isTintable = false)
} else {
TextFieldIcon(CardBrand.Unknown.icon, isIcon = false)
val staticIcons = cardBrands.map { cardBrand ->
TextFieldIcon.Trailing(cardBrand.icon, isTintable = false)
}.filterIndexed { index, _ -> index < 3 }

val animatedIcons = cardBrands.map { cardBrand ->
TextFieldIcon.Trailing(cardBrand.icon, isTintable = false)
}.filterIndexed { index, _ -> index > 2 }

TextFieldIcon.MultiTrailing(
staticIcons = staticIcons,
animatedIcons = animatedIcons
)
}
}

Expand Down
2 changes: 1 addition & 1 deletion payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CvcController.kt
View file
Open in desktop
Expand Up @@ -68,7 +68,7 @@ internal class CvcController constructor(
}

override val trailingIcon: Flow<TextFieldIcon?> = cardBrandFlow.map {
TextFieldIcon(it.cvcIcon, isIcon = false)
TextFieldIcon.Trailing(it.cvcIcon, isTintable = false)
}

override val loading: Flow<Boolean> = MutableStateFlow(false)
Expand Down
4 changes: 2 additions & 2 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/IbanConfig.kt
View file
Open in desktop
Expand Up @@ -30,9 +30,9 @@ class IbanConfig : TextFieldConfig {
override val keyboard = KeyboardType.Ascii

override val trailingIcon: MutableStateFlow<TextFieldIcon?> = MutableStateFlow(
TextFieldIcon(
TextFieldIcon.Trailing(
R.drawable.stripe_ic_bank_generic,
isIcon = true
isTintable = true
)
)
override val loading: StateFlow<Boolean> = MutableStateFlow(false)
Expand Down
49 changes: 14 additions & 35 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/RowElementUI.kt
View file
Open in desktop
@@ -1,15 +1,18 @@
package com.stripe.android.ui.core.elements

import android.content.res.Resources
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.width
import androidx.compose.material.Divider
import androidx.compose.material.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.layout.onSizeChanged
import androidx.compose.ui.unit.dp
import androidx.constraintlayout.compose.ConstraintLayout
import androidx.constraintlayout.compose.Dimension
import com.stripe.android.ui.core.paymentsColors
import com.stripe.android.ui.core.paymentsShapes

Expand All @@ -21,54 +24,30 @@ internal fun RowElementUI(
lastTextFieldIdentifier: IdentifierSpec?
) {
val fields = controller.fields

val numVisibleFields = fields.filter { !hiddenIdentifiers.contains(it.identifier) }.size

val dividerHeight = remember { mutableStateOf(0.dp) }
// Only draw the row if the items in the row are not hidden, otherwise the entire
// section will fail to draw
if (fields.map { it.identifier }.any { !hiddenIdentifiers.contains(it) }) {
// An attempt was made to do this with a row, and a vertical divider created with a box.
// The row had a height of IntrinsicSize.Min, and the box/vertical divider filled the height
// when adding in the trailing icon this broke and caused the overall height of the row to
// increase. By using the constraint layout the vertical divider does not negatively effect
// the size of the row.
ConstraintLayout {
// Create references for the composables to constrain
val fieldRefs = fields.map { createRef() }
val dividerRefs = fields.map { createRef() }

Row(modifier = Modifier.fillMaxWidth()) {
fields.forEachIndexed { index, field ->
SectionFieldElementUI(
enabled,
field,
hiddenIdentifiers = hiddenIdentifiers,
lastTextFieldIdentifier = lastTextFieldIdentifier,
modifier = Modifier
.constrainAs(fieldRefs[index]) {
if (index == 0) {
start.linkTo(parent.start)
} else {
start.linkTo(dividerRefs[index - 1].end)
}
top.linkTo(parent.top)
.weight(1.0f / numVisibleFields.toFloat())
.onSizeChanged {
dividerHeight.value =
(it.height / Resources.getSystem().displayMetrics.density).dp
}
.fillMaxWidth(
(1f / numVisibleFields.toFloat())
)
)

if (!hiddenIdentifiers.contains(field.identifier) && index != fields.lastIndex) {
if (index != fields.lastIndex) {
Divider(
modifier = Modifier
.constrainAs(dividerRefs[index]) {
start.linkTo(fieldRefs[index].end)
top.linkTo(parent.top)
bottom.linkTo(parent.bottom)
height = (Dimension.fillToConstraints)
}
.padding(
horizontal = MaterialTheme.paymentsShapes.borderStrokeWidth.dp
)
.height(dividerHeight.value)
.width(MaterialTheme.paymentsShapes.borderStrokeWidth.dp),
color = MaterialTheme.paymentsColors.componentDivider
)
Expand Down
27 changes: 18 additions & 9 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/TextFieldController.kt
View file
Open in desktop
Expand Up @@ -35,15 +35,24 @@ interface TextFieldController : InputController {
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
data class TextFieldIcon(
@DrawableRes
val idRes: Int,
@StringRes
val contentDescription: Int? = null,

/** If it is an icon that should be tinted to match the text the value should be true */
val isIcon: Boolean
)
sealed class TextFieldIcon {
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
data class Trailing(
@DrawableRes
val idRes: Int,
@StringRes
val contentDescription: Int? = null,

/** If it is an icon that should be tinted to match the text the value should be true */
val isTintable: Boolean
) : TextFieldIcon()

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
data class MultiTrailing(
val staticIcons: List<Trailing>,
val animatedIcons: List<Trailing>
) : TextFieldIcon()
}

/**
* This class will provide the onValueChanged and onFocusChanged functionality to the field's
Expand Down