-
Notifications
You must be signed in to change notification settings - Fork 161
allow-custom-billing-address-specifications #3200
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
base: v5
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3cd1f02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| const showBillingAddress = props.billingAddressMode !== AddressModeOptions.none && props.billingAddressRequired; | ||
|
|
||
| const partialAddressSchema = handlePartialAddressMode(props.billingAddressMode); | ||
| const partialAddressSchema = handlePartialAddressMode(props.billingAddressMode, specifications.getSpecifications()); |
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 will allow us to use the custom specifications properly
| showStoreDetailsCheckbox?: boolean; | ||
| showWarnings?: boolean; | ||
| specifications?: Specifications; | ||
| specifications?: AddressSpecifications; |
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.
fixed the type here, the value of specifications coming from props is not the class rather its the AddressSpecifications object. This was not caught by TS since in specifications.ts class we have the following logic:
class Specifications {
private specifications: AddressSpecifications;
constructor(specifications?) {
this.specifications = { ...ADDRESS_SPECIFICATIONS, ...specifications };
}
here using specifications? in the constructor, assumes specifications to be type any
|
|
||
| export const handlePartialAddressMode = (addressMode: AddressModeOptions): AddressSpecifications | null => { | ||
| export const handlePartialAddressMode = (addressMode: AddressModeOptions, specifications: AddressSpecifications): AddressSpecifications | null => { | ||
| if (specifications) { |
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 is the main addition to this MR, in case we get specifications, we can prefer it over addressMode
|
|
||
| class Specifications { | ||
| private specifications: AddressSpecifications; | ||
| private readonly specifications: AddressSpecifications; |
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.
added for safety
| constructor(specifications?) { | ||
| this.specifications = { ...ADDRESS_SPECIFICATIONS, ...specifications }; | ||
| constructor(specifications?: AddressSpecifications) { | ||
| if (specifications && typeof specifications === 'object') { |
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.
add a small check here to make sure the input type is correct
| * Returns the specifications object. | ||
| * @returns specifications - returns combines specifications | ||
| */ | ||
| getSpecifications(): AddressSpecifications { |
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.
added this new getter to give the merged specification
|
Can you please guide me to find what is the best approach here to add this change to both V5 and V6? |
|
Hey @M-Izadmehr If I understand correctly the objective of the PR is to allow passing |
Hey @m1aw, exactly. In our case, we noticed our users from some countries are not used to having some of the fields. In this case for example for
|
|
@M-Izadmehr thank you for the PR. We agree that this is a feature we want to expose, we aren't certain if this is something we want to expose in it's current form. We had some changes to address component in the pipeline, we want to ensure this feature is compatible with this. So for now we are parking this change. We will try to revisit ASAP. |
|
Hi @m1aw, |
Summary
Adyen-web drop-in can get
specificationswhich can provide address specifications. This can be used to specifically determine for each country, which billing address fields are needed, and what are specific details needed for each field (label/placeholder/optional/etc).However, in the code, the
specificationsis not actively used, and finally, when rendering the Credit Card Address component, the main criteria depend onbillingAddressModeonly. And the specifications are ignored.Here is a summary of the data flow:
CardInput.tsx(github). While the specifications are used they are not passed toFieldsToRenderedFieldsToRenderusesCardFieldsWrapper:CardFieldsWrapperwill use thepartialAddressSchemaasspecificationsand completely ignores specifications you pass in (github)so it either uses the hardcoded partial form (meaning only postal code) or
null.This MR will ensure that if address specifications are passed, they are preferred over default specifications.
This means if you pass
specificationsto credit card config when initializing the drop-in like this:Tested scenarios
Fixed issue: