Replace the payment dependency with credit-card-type and luhn#77
Replace the payment dependency with credit-card-type and luhn#77cassiocardoso wants to merge 14 commits intomasterfrom
Conversation
| @@ -0,0 +1,35 @@ | |||
| export const dankort = { | |||
There was a problem hiding this comment.
I created these 3 definitions here while they aren't supported by credit-card-type
There was a problem hiding this comment.
As we discussed, use this file (with a different name) to abstract the usage of credit-card-type and luhn so it will be easy to replace it in the future if we need to.
There was a problem hiding this comment.
I moved this file to /utils.
Initially, I had the helpers and these objects in the same file, but it was a bit confusing for me, so I decided to split into 2 files just to be easier to understand what's being done in each of them.
Let me know what do you think.
src/index.js
Outdated
| if (number && !preview) { | ||
| const validatedIssuer = creditCardType(sanitizeNumber(number)).length ? creditCardType(sanitizeNumber(number))[0].type : 'unknown'; | ||
|
|
||
| if (validCardTypes.includes(validatedIssuer)) { | ||
| updatedIssuer = validatedIssuer; | ||
| } | ||
| } |
There was a problem hiding this comment.
I had to tweak the logic here since creditCardType will always return an array of card type objects.
Also added the option to bypass this logic if the preview prop is passed, in this case, we use the issuer prop
There was a problem hiding this comment.
Another reason for the abstraction.
Get the issuer only if there's a single item in the array.
and we already have an issuer getter that handles that.
There was a problem hiding this comment.
got your point on the issuer getter, makes total sense, reverted the logic here.
But I noticed one thing with this part:
Get the issuer only if there's a single item in the array.
In this case, we get a lot more unknowns, for example:
| number | type |
|---|---|
| 4 | unknown |
| 40 | unknown |
| 401 | unknown |
| 4011 | unknown |
| 40117 | unknown |
| 401176 | visa |
| 401178 | elo |
Before we got visa from 4 until 40117, and only switched to elo if the bin matched. Do you think this could be an issue?
gilbarbara
changed the title
gilbarbara
changed the title
| @@ -0,0 +1,35 @@ | |||
| export const dankort = { | |||
There was a problem hiding this comment.
As we discussed, use this file (with a different name) to abstract the usage of credit-card-type and luhn so it will be easy to replace it in the future if we need to.
src/index.js
Outdated
| if (number && !preview) { | ||
| const validatedIssuer = creditCardType(sanitizeNumber(number)).length ? creditCardType(sanitizeNumber(number))[0].type : 'unknown'; | ||
|
|
||
| if (validCardTypes.includes(validatedIssuer)) { | ||
| updatedIssuer = validatedIssuer; | ||
| } | ||
| } |
There was a problem hiding this comment.
Another reason for the abstraction.
Get the issuer only if there's a single item in the array.
and we already have an issuer getter that handles that.
| $rccs-animate-background: true; | ||
|
|
||
| /** ISSUERS **/ | ||
| $rccs-amex-background: linear-gradient(25deg, #308c67, #a3f2cf) !default; |
There was a problem hiding this comment.
we might need to revert these
| focused: 'number', | ||
| }); | ||
|
|
||
| expect(wrapper.find('.rccs__card').hasClass('rccs__card--amex')).toBe(true); |
There was a problem hiding this comment.
The same thing here, we might need to revert these changes
There was a problem hiding this comment.
I'm not sure if I got your point here, but given the newly created cardTypesMap:
export const cardTypesMap = {
amex: ['amex', 'americanexpress', 'american-express'],
dinersclub: ['diners', 'dinersclub', 'diners-club'],
visaelectron: ['visaelectron', 'visa-electron'],
};Should we use the object keys as our identifiers for the CSS class?
- use credit-card-type + luhn to achieve the same functionality previously provided by the payment lib - some types had their names updated to match the new lib pattern (american-express and diners-club)
- add new card type information - update local development section
- use class getter and setter to store the validCardTypes value instead of using the component state - refactor config function name to be more explicit about its implementation
- fix issue reported by DeepScan
cassiocardoso
force-pushed
the
feature/remove-payment
branch
from
713b405 to
2e18b7f
Compare
|
any progress in this pr? |
|
@cassiocardoso and @gilbarbara was there anything left outstanding on this it'd be super cool to see this unit finished :) |
|
This PR seems to be very useful. Any estimative when it will be merged it with master? |
5 participants
Proposal
Remove
Paymentdependency in favor ofcredit-card-type+luhn.Closes #75
Details
After removing Payment, we needed to store the list of supported card types inside the library, I chose to do that using the internal component state.
Some small tweaks were needed since the libs identified the card types differently:
No changes were made to the component API.
Caveats
Currently, Dankort is not supported by
credit-card-typewhich led to some inline changes here to make it work.If/when this PR is approved: braintree/credit-card-type#106 we'll be able to remove these changes on our side and rely on it