mobify / pinny

A mobile-first content fly-in UI plugin
MIT License
23 stars 4 forks source link

Migrate to jQuery & NPM #93

Closed marlowpayne closed 8 years ago

marlowpayne commented 8 years ago

Reviewers: @fractaltheory @jansepar @mikenikles @MikeKlemarewski

Changes

mikenikles commented 8 years ago

Nice work, @marlowpayne!

I agree that we're looking at a major version bump here. Should we add a note to the README to let people know Zepto support is available up until v2.0.3? In case someone relies on Zepto and wants to use Pinny.

MikeKlemarewski commented 8 years ago

This is looking pretty great! My one question would be: what happens when I require in Pinny to my adaptive project and they depend on different jQueries? I bet I'll get two jQueries in my bundle which seems a bit wasteful. It would be sweet if we could give the option to BYO jQuery

marlowpayne commented 8 years ago

Great question @MikeKlemarewski ! Someone please correct me if I'm wrong here, but I am pretty sure Pinny is already flexible to the version of jQuery that it binds to. We first bind to require's version of $ (which can be set from the project's require config), but failing that we bind $ to window's Zepto or jQuery.

The only time we bind to our NPM version of jQuery is when we set up our examples and tests.

highruned commented 8 years ago

That's correct @marlowpayne. Well, almost. If it's being built in an AMD environment (eg. our Require.js builds), our standard plugins bring in $ as whatever package we define $ to be, which is usually jQuery and used to be Zepto. If you don't define a $, it's not going to fall back, it's going to error out. If it's being run in the browser with no AMD environment (it is possible you could with browserify, etc.), we assume you are bringing in your own selector library and have a window.Zepto or window.jQuery defined.

I'm wondering if we might as well just drop that window.Zepto check, or at least make it secondary to the window.jQuery check?

marlowpayne commented 8 years ago

@ericmuyser, since this'll be a major version bump I'm fine with scrubbing all checks and references to Zepto in the code.

marlowpayne commented 8 years ago

Let's test out these changes on an in-flight project using jQuery and Pinny.

highruned commented 8 years ago

I'm cool with us using it in our project if you want to put it in sometime this week @marlowpayne :+1:

jansepar commented 8 years ago

:+1: ! If we haven't already, let's prep a release (and remember to use our branching strategy to do so :) )

mikenikles commented 8 years ago

:+1: from me as well. I tested that on a current project and confirm the upgrade works.