search

powmedia / backbone-forms

Form framework for BackboneJS with nested forms, editable lists and validation
MIT License
2.17k stars 413 forks source link

Replace use of jQuery global $ with Backbone.$ #529

Closed glenpike closed 7 years ago

glenpike commented 8 years ago

Fixes #321. Add tests where appropriate to catch regressions.

philfreo commented 7 years ago

It doesn't look like you've found/replaced all instances of $ in src/

Also, can't we do a single var $ = Backbone.$ line at the top of each source file rather than finding it in every function? The seems like it'd be fine since the distribution files are all wrapped in an IIFE.

However, you also have to think of the AMD distribution files, which explicitly require jquery. The AMD template could also be potentially changed so that it removes that dependency, and each module instead just looks at Backbone.$

glenpike commented 7 years ago

I've checked through src/ again for instances of '$' that are not referenced to Backbone.$, but I can't see any. If you can see any specifics, can you let me know?

Regarding single var $ = Backbone.$ in each source file - when these are all wrapped in the IIFE we would be redefining $ each time - which isn't an error, but seems a bit messy to me - just using a var near where it's needed is my preference.

The AMD issue. We'd have a similar issue with Underscore vs Lodash. As Backbone.js does the same with it's AMD module (https://github.com/jashkenas/backbone/blob/master/backbone.js#L17) I'm happy to keep it this way as there are a number of links online about using Zepto instead of jQuery / Lodash instead of Underscore. It might be good to add links in the Wiki / README, so I'll consider that.

philfreo commented 7 years ago

Sorry - I think I was searching through the wrong version when I was looking for other instances. I can't find any missing $ references now.

Overall LGTM then. 👍