Add shipping details to payment sheet #5419
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameswoo-stripe
requested review from
brnunes-stripe and
michelleb-stripe
as code owners
jameswoo-stripe
commented
Aug 16, 2022
| ?.takeIf { loadFromArgs } | ||
| ?.let { convertToFormValuesMap(it) } | ||
| ?: emptyMap() | ||
| val initialValuesMap = args.initialValueMap |
There was a problem hiding this comment.
Should loadFromArgs be used here?
There was a problem hiding this comment.
loadFromArgs exists because when you launch Link passing in the pre-filled card params, you'll want the form to be pre-filled with those values. But then if the user navigates within Link and ends up in this screen again, we won't want to load the values from the initial arguments. How does this logic apply to address?
jameswoo-stripe
commented
Aug 16, 2022
| CountryConfig(this.allowedCountryCodes), | ||
| initialValue = initialValues[this.apiPath] | ||
| ) | ||
| ): SectionElement { |
There was a problem hiding this comment.
Attempted to convert the AddressElement into a FormElement, but this became a problem when getting the hidden identifiers. FormElement doesn't have a way to access its children like SectionElement does. We should refactor the way elements works. Ideally, the Element itself always has access to its children.
There was a problem hiding this comment.
Here, I am creating a new SectionElement with two fields: AddressElement and SameAsShippingElement
jameswoo-stripe
commented
Aug 16, 2022
| @@ -7,6 +7,8 @@ import kotlinx.coroutines.flow.Flow | |||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | |||
| sealed interface SectionFieldElement { | |||
| val identifier: IdentifierSpec | |||
| val shouldRenderOutsideCard: Boolean | |||
There was a problem hiding this comment.
This was needed for SectionElementUI, this lets the section know if this element should be rendered outside of the form card. Otherwise all elements get rendered in the card.
jameswoo-stripe
commented
Aug 16, 2022
| @@ -401,6 +402,21 @@ class PaymentSheetPlaygroundActivity : AppCompatActivity() { | |||
| customer = viewModel.customerConfig.value, | |||
| googlePay = googlePayConfig, | |||
| defaultBillingDetails = defaultBilling, | |||
| shippingDetails = if (viewModel.getSavedToggleState().setShippingAddress) { | |||
There was a problem hiding this comment.
If the shipping address toggle is enabled, you can test this feature without going through the autocomplete flow.
jameswoo-stripe
commented
Aug 16, 2022
| * The field is displayed and the customer is required to fill it in. | ||
| */ | ||
| REQUIRED | ||
| data class AdditionalFieldsConfiguration( |
There was a problem hiding this comment.
Updating the structure of AddtionalFieldsConfiguration to match iOS
There was a problem hiding this comment.
Do we need a @JvmOverloads for Java?
|
Diffuse output: APKDEXARSC |
jameswoo-stripe
force-pushed
the
jameswoo/shipping-address-element-interop
branch
from
c0077a4
to
0ba5b1d
Compare
jameswoo-stripe
force-pushed
the
jameswoo/shipping-address-element-interop
branch
from
0ba5b1d
to
87daf10
Compare
brnunes-stripe
suggested changes
Aug 17, 2022
There was a problem hiding this comment.
How does this change look like for a returning customer, where we would show the "Save for future use" checkbox? Does it show two checkboxes?
I'm also thinking if the checkbox being all the way in the bottom is the best UX. The user might start filling in the address and not be aware of the checkbox until the end.
| @@ -11,6 +12,7 @@ interface LinkPaymentLauncherFactory { | |||
| fun create( | |||
| @Assisted(MERCHANT_NAME) merchantName: String, | |||
| @Assisted(CUSTOMER_EMAIL) customerEmail: String?, | |||
| @Assisted(CUSTOMER_PHONE) customerPhone: String? | |||
| @Assisted(CUSTOMER_PHONE) customerPhone: String?, | |||
| @Assisted(INITIAL_VALUES_MAP) initialValuesMap: Map<IdentifierSpec, String?>? | |||
There was a problem hiding this comment.
Can you make the names more descriptive? It's not clear what's in the map. Something like INITIAL_SHIPPING_ADDRESS_VALUES_MAP, initialShippingAddressValuesMap is clearer.
There was a problem hiding this comment.
Or if you're thinking of making this generic (not only for shipping address), maybe initialFormValuesMap
There was a problem hiding this comment.
Sure, I can make it less generic
I like initialFormValuesMap
| } else { | ||
| emptyMap() | ||
| } | ||
| return@let it.create( |
There was a problem hiding this comment.
I think the return@let is not needed.
| * The field is displayed and the customer is required to fill it in. | ||
| */ | ||
| REQUIRED | ||
| data class AdditionalFieldsConfiguration( |
There was a problem hiding this comment.
Do we need a @JvmOverloads for Java?
jameswoo-stripe
force-pushed
the
jameswoo/shipping-address-element-interop
branch
from
3bbdccb
to
1e16ed1
Compare
brnunes-stripe
previously approved these changes
Aug 18, 2022
payments-ui-core/src/main/java/com/stripe/android/ui/core/address/ShippingAddress.kt
Show resolved
Hide resolved
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/SameAsShippingElementUI.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentsheet/addresselement/AddressLauncher.kt
Outdated
Show resolved
Hide resolved
jameswoo-stripe
force-pushed
the
jameswoo/shipping-address-element-interop
branch
from
1e16ed1
to
9bf4324
Compare
jameswoo-stripe
assigned brnunes-stripe and tillh-stripe and unassigned jameswoo-stripe
brnunes-stripe
approved these changes
Aug 18, 2022
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
shippingDetailstoPaymentSheet.ConfigurationSameAsShippingElement, a new checkbox element that will be shown below theAddressElementwhenever theshippingDetailsare supplied, default billing details is not enabled, and the merchant has configured for this checkboxShipping details is parsed into the initial values map to be used for the billing details. If customer requests same as shipping, then the billing details will be overwritten with the shipping information.
The checkbox is there if and only if the merchant has supplied the necessary configurations (shipping address config):
This is enabled for Link add payment method screen and payment sheet, otherwise you should not see this new checkbox.
Motivation
Address Element private beta
Testing
Screenshots
shippingdetails.mov