-
Notifications
You must be signed in to change notification settings - Fork 255
Comments
Issue #2790551 by steveoliver, bojanz, borisson_: Implement a JS libr...#663
Issue #2790551 by steveoliver, bojanz, borisson_: Implement a JS libr... #663mglaman wants to merge 1 commit intodrupalcommerce:8.x-2.x from
Conversation
6be1d5c to
b8a7444
Compare
5b3779b to
2941d08
Compare
53ec70a to
3bc1b88
Compare
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
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()?
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.
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.
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.
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.
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.
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;
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?
bojanz
commented
Feb 7, 2018
This PR needs work. We basically want a JS version of our CreditCard.php code.
In comparison to it, this JS code lacks certain credit card types (JCB, UnionPay) and Luhn validation.
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
807c6e8 to
8898142
Compare
...ary for the credit card form