Skip to content
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

Update jquery payment js library #608

Merged
merged 2 commits into from
Dec 23, 2015
Merged

Update jquery payment js library #608

merged 2 commits into from
Dec 23, 2015

Conversation

mtomov
Copy link
Contributor

@mtomov mtomov commented Dec 19, 2015

  • Update old version from 3 Sep 2013 to the latest release.

  • includes lots of bug & browser compatibility fixes

  • Specify type="tel" for numerical input fields, which fixes
    non-working card expiry field on Samsung android phones

    Follows library recommendations


Second commit is a bit controversial, so you might consider excluding it.

Autocomplete name on the card and card number, but forbid CVV autocomplete.

  * As per the autocomplete recommendations at:
    https://github.com/stripe/jquery.payment#autocomplete-recommendations

This _gateway is anyhow overridden by the gateways, so the autocomplete might not matter;

On the other hand, the addition of the type=tel attribute is indeed important to fix annoying bugs on Samsung android phones, which occur without it (even with the latest version of the jquery.payment library).

@cbrunsdon
Copy link
Contributor

This looks good and reasonable to me, I'm 👍

Someone else might want to complain about the switch to coffee though? I usually include .min's on my vendored assets but don't have strong opinions either way.

@jhawthorn
Copy link
Contributor

I don't feel strongly about the .min.js. It's nice to have the .js to have our development slightly faster since we don't have to transpile, but the coffee is nice and readable. I'm fine either way.

A short comment in _gateway.html.erb linking to the jquery.payment docs to explain type=tel would be nice, but maybe the commit message is sufficient.

Either way 👍

@jhawthorn
Copy link
Contributor

For when I later compile release notes: This was originally introduced here 5c400ac (7 Feb 2013) so the existing stripe.js version would have been v1.0 and this upgrades us to v1.3.0

@athal7
Copy link

athal7 commented Dec 23, 2015

I also find it a bit odd to have a vendored asset in coffeescript, usually use the minified version.

@tvdeyen
Copy link
Member

tvdeyen commented Dec 23, 2015

I also don't like to have vendored assets as Coffescript file. I would add the unminified JS version. AFAIK sprockets does not skip already minified files and it is great to have the readable version for debugging.

@athal7
Copy link

athal7 commented Dec 23, 2015

@tvdeyen that sounds great to me

@mtomov
Copy link
Contributor Author

mtomov commented Dec 23, 2015

I could certainly change it to whichever one you prefer. The coffee script
version came when I downloaded the latest Released, thereby. Other versions
are here https://github.com/stripe/jquery.payment/tree/master/lib.

On Wed, Dec 23, 2015 at 8:00 PM, Thomas von Deyen notifications@github.com
wrote:

I also don't like to have vendored assets as Coffescript file. I would add
the unminified JS version. AFAIK sprockets does not skip already minified
files and it is great to have the readable version for debugging.


Reply to this email directly or view it on GitHub
#608 (comment).

  * Update old version from 3 Sep 2013 to the latest release.

  * includes lots of bug & browser compatibility fixes

  * Specify type="tel" for numerical input fields, which fixes
    non-working card expiry field on
    [Samsung android phones](http://github.com/stripe/jquery.payment/issues/112)

    Follows library [recommendations](https://github.com/stripe/jquery.payment#mobile-recommendations)
@mtomov
Copy link
Contributor Author

mtomov commented Dec 23, 2015

Should be good now.

@tvdeyen
Copy link
Member

tvdeyen commented Dec 23, 2015

Ah. A proper diff 🤗 Thanks @mtomov

@athal7
Copy link

athal7 commented Dec 23, 2015

👍

jhawthorn added a commit that referenced this pull request Dec 23, 2015
@jhawthorn jhawthorn merged commit 3050f0a into solidusio:master Dec 23, 2015
@jhawthorn
Copy link
Contributor

Thanks Martin 🎉 💳

@mtomov
Copy link
Contributor Author

mtomov commented Dec 24, 2015

👍

@mtomov mtomov deleted the update-jquery-payment-js-library branch December 24, 2015 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants