search

jondavidjohn / payform

:credit_card: A library for building credit card forms, validating inputs, and formatting numbers.
https://jondavidjohn.github.io/payform/
Other
427 stars 81 forks source link

Incorporate changes upstream #1

Closed jamesreggio closed 9 years ago

jamesreggio commented 9 years ago

I'm glad to see you giving a little love to the jquery.payment codebase, but confused as to why you didn't propose these changes in a Pull Request against the original repository.

I'm sure other users would benefit from the dependency reduction and addition of UMD, and we would certainly be willing to take improvements to the logistical pieces of the repo (such as the Travis CI automation you added).

Would you consider packaging these up into a PR?

jondavidjohn commented 9 years ago

Not against helping to incorporate the changes upstream at all, just assumed that the jquery.payment project was firmly positioned as a jQuery Plugin specifically.

My changes seem to go against that.

jondavidjohn commented 9 years ago

@jamesreggio Was that an incorrect assumption?

jamesreggio commented 9 years ago

Sorry about the delay, @jondavidjohn—GitHub notifications were going to the wrong email account.

I think the ideal module would be something that doesn't require jQuery, but exposes the existing jQuery plugin API for compatibility. (In support of this ideal, I recently cut out some jQuery-specific dependencies in order to support Zepto users.)

The decision to release this module as a jQuery plugin made a little more sense in 2012 than it does today, but now I view the jQuery interface as a convenience for some, and an adoption blocker for others. If we placed a compatibility shim over your de-jQueryified version, it would be the best of both worlds.

If you're interested in doing that work, we'd certainly appreciate it. Otherwise, it may be something I'll get around to doing in the future. (It's my humble opinion that the architecture of jquery.payment is terrible, and I'd like to rework it using some newer DOM APIs, like setRangeText).

jamesreggio commented 9 years ago

In either case, though, would you mind restoring Line 1 to the LICENSE (with your new copyright for modifications as Line 2)?

jondavidjohn commented 9 years ago

I think the jQuery plugin shim over the top seems like the right solution, I would be interested in facilitating that any way I can.

And apologies for the licensing issue, I hope it's apparent that I want to give the proper credit.

The commit referenced above in develop should correct that.

nickpresta commented 9 years ago

I'm mega +1 to this collaboration and getting in the "non-jQuery core" and then wrapping it with the jQuery plugin interface.

I was going to do something very similar (remove the jQuery dependency) but found this before I started.

jondavidjohn commented 9 years ago

A real basic proof of concept using browserify to build it.

http://requirebin.com/?gist=5ff6ab7a58c4036e1886

jondavidjohn commented 9 years ago

@jamesreggio If I were to implement jquery.payform as discussed above with close api compatibility with jquery.payment and include it in this repository (because it's likely a mainline use case), are you suggesting that this would be considered the continuation of jquery.payment? Or are you suggesting that jquery.payment move to simply consume payform?

Making them separate projects seems to complicate releases, etc...

jamesreggio commented 9 years ago

I'm very sorry to have swooped in with an opinion, and then become horribly unresponsive.

My initial instinct is to subsume this work back into jquery.payment. The repo name becomes a bit anachronistic, but it's still a very strong brand, and I think we could clearly communicate in the README that jQuery is, in fact, not required. If you're okay with that, I think we could make you a collaborator on the repo, to expedite your future enhancements.

Alternatively, we could gut jquery.payment of everything but the jQuery interface, and then direct it to use payform as a dependency. I'll have to talk to my colleagues about that, because I have a little bit of concern about making a Stripe-hosted repo basically a simple facade for another person's code.

(I'm concerned that somebody might commit code that publishes CC info in a network request. People have proposed patches that essentially do this in the past, but not with malicious intent.)

Let me know how you feel about the first option, and I'll speak to everybody tomorrow about the second.

Thanks again for your patience.

jondavidjohn commented 9 years ago

I agree that having a separate repository for the jquery shim is not ideal. Especially since I've already got one I've put together in this repo :smile:.

Yeah, I don't know about putting it back under jquery.payment. Seems to put us in a weird spot because I've done a bit of work to get it out of the jQuery box, and it would seem like a step back to put it back under the jquery.payment name, maybe it's irrational but I believe many people see the name and pass (rightly) assuming that it's only consumable as a jquery plugin. Thoughts on moving it as payform under the Stripe org?

jondavidjohn commented 9 years ago

On the idea that jquery.payment is a strong brand, I would clearly agree, but I think much of that can be transferred assuming the pivoted project is made apparent in the jquery.payment readme.

It seems like that makes more long term sense as opposed for something that is not a jquery plugin, having a jquery plugin style name.

jondavidjohn commented 9 years ago

@jamesreggio :grimacing:

jondavidjohn commented 9 years ago

@jamesreggio Let me know if you have any further thoughts on this, closing for now.

PavelGavlik commented 9 years ago

It would awesome to see Stripe-supported vanilla JS version of jquery.payment. I hope you guys make it happen somehow.

jondavidjohn commented 9 years ago

@mduvall with a new maintainer, any renewed interest in the suggestions outlined above? :point_up_2: