-
Notifications
You must be signed in to change notification settings - Fork 257
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
Issue #2790551 by steveoliver, bojanz, borisson_: Implement a JS libr… #663
base: 8.x-2.x
Are you sure you want to change the base?
Issue #2790551 by steveoliver, bojanz, borisson_: Implement a JS libr… #663
Conversation
6be1d5c
to
b8a7444
Compare
'0604', | ||
'6390' | ||
], | ||
lenghts: [12, 13, 14, 15, 16, 17, 18, 19] |
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.
Should be lengths
var MAESTRO = 'maestro'; | ||
|
||
types[VISA] = { | ||
niceType: '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.
Why niceType and not label?
And why aren't the labels passed through Drupal.t()?
niceType: 'Visa', | ||
type: VISA, | ||
pattern: ['4'], | ||
gaps: [4, 8, 12], |
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.
Gaps are not used anywhere, and don't exist on the PHP side, let's remove them.
types[MASTERCARD] = { | ||
niceType: 'MasterCard', | ||
type: MASTERCARD, | ||
pattern: ['51-55', '222100-272099'], |
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.
"pattern" should be "prefixes" to match the PHP code.
var AMEX = 'amex'; | ||
var DINERSCLUB = 'dinersclub'; | ||
var DISCOVER = 'discover'; | ||
var MAESTRO = 'maestro'; |
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.
We're missing JCB and UnionPay.
* @return {object|boolean} | ||
*/ | ||
var detectType = function (number) { | ||
// Loop over all available types. |
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.
Comments that retell what the code is doing ("Loop over all the available types") aren't useful, let's remove them.
|
||
// Loop over all patterns in the type. | ||
for (var i in type.pattern) { | ||
var pattern = type.pattern[i]; |
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.
Why don't we introduce a matchPrefix() helper like we did in PHP?
On the other hand, validatePrefix() could probably be inlined.
var range; | ||
exploded_pattern = pattern.split('-'); | ||
|
||
while (exploded_pattern[0] <= exploded_pattern[1]) { |
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 feels overly complex.
We reimplementing the following code:
list($start, $end) = explode('-', $prefix);
$number = substr($number, 0, strlen($start));
return $number >= $start && $number <= $end;
} | ||
} | ||
|
||
ValidationDiv.append('CC is of type: ' + type.niceType); |
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 looks like debug code?
This PR needs work. We basically want a JS version of our CreditCard.php code. Compared to 11 months ago, we have a real world need for the code in Commerce AuthNet: https://www.drupal.org/project/commerce_authnet/issues/2932486 which should help us ensure that the merged code is solid. Also, ouch, we have no way to write JS unit tests yet. Issue to follow is https://www.drupal.org/project/drupal/issues/2815199 |
…ary for the credit card form
d324fa2
to
48e9208
Compare
…ary for the credit card form