Fix #11464 - JS noConflict() mode not working in 3.0.x

[colllin]

Refactor tests to run everything in noConflict mode. This is a philosophy change, from "We're testing a jQuery plugin" to "We're testing a library that depends on jQuery". Then, almost as an afterthought, there is an additional test to make sure the library correctly gets attached as a jQuery plugin.

Refactor all plugins to use an internal Plugin reference to the plugin function. The plugins can't expect to be attached to the jQuery object when in noConflict mode.

[cvrebert]

@colllin Part of the test suite is failing; see https://travis-ci.org/twbs/bootstrap/jobs/16172124

[colllin]

Bad merge. Thanks for the heads up. I'll look at it tomorrow.

On Tue, Dec 31, 2013 at 12:49 AM, Chris Rebert [email protected]:

@colllin https://github.com/colllin Part of the test suite is failing;
see https://travis-ci.org/twbs/bootstrap/jobs/16172124

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11966#issuecomment-31386785
.

[colllin]

Hey, just to be clear, this is what I did:

1 - I fixed a few js bugs affecting noConflict mode.
2 - I refactored ALL js tests to use the plugins in noConflict mode. That means every plugin call like this:

var div = $('.my-selector')
div.carousel('prev')

becomes something like this:

var div = $('.my-selector')
_carousel(div, 'prev')

... with setup and teardown happening in the test module around each plugin. Testing the plugins in noConflict mode is the only way to ensure that the plugins work in noConflict mode, but once we test it in noConflict mode, it seems redundant to test again in normal mode (as long as we check once at the beginning to make sure that the jQuery plugin got attached correctly).

So, my questions are:
1 - How do we message this testing situation to other contributors?
2 - Would it be worth refactoring all the JS to be a standalone library that depends on jQuery, and happens to attach itself as a set of jQuery plugins? The code change would be small, but the conceptual change might make the testing seem natural instead of backward -- instead of detaching a jQuery plugin and testing it with unfamiliar function names and an unfamiliar syntax, you would just be testing a JS suite and then making sure it's attached as a jQuery plugin at the end. So there might be a global JS namespace for accessing the bootstrap plugins, for example:

Bootstrap(div).carousel('prev')

or

Bootstrap.carousel(div, 'prev')

or even

Bootstrap.carousel(div).prev()

[cvrebert]

Regarding messaging, we should add a statement about it to CONTRIBUTING.md.

[cvrebert]

/cc @fat

[zlatanvasovic]

@cvrebert @colllin This breaks #12058, as it doesn't use right indent.

[fat]

spacing looks a bit weird and the = aren't lined up… so it doesn't really match the style of these plugins

[fat]

I think this is a good idea… there are some little things that should be cleaned up (lots of style stuff), but in general its good work. thanks a lot @colllin

[fat]

could you change these to:

  var old = $.fn.affix

  $.fn.affix             = Plugin
  $.fn.affix.Constructor = Affix

[colllin]

@fat There was a huge style refactor since I originally submitted this pull request. I will update to match the new style.

[fat]

awesome, thanks man

[cvrebert]

@colllin Friendly ping on this.

[colllin]

Thanks @cvrebert . I needed the push, and I happened to have time tonight.

Started over with a fresh branch because of all the changes since I originally submitted.

[cvrebert]

@colllin Awesome! <3

[cvrebert]

Passes Sauce cross-browser unit tests: https://travis-ci.org/twbs/bootstrap/jobs/23485897

[cvrebert]

@fat Could you re-review this new revision? (And sorry about the number of pings lately. Just trying to keep things moving forward.)

[fat]

yep no problem… will review now, and try to catch up this weekend

[fat]

this lgtm

[fat]

@colllin thanks so much for this ❤️ great work!

[colllin]

thanks! happy to help :)

On Fri, Apr 25, 2014 at 4:44 PM, Jacob [email protected] wrote:

@colllin https://github.com/colllin thanks so much for this [image:
❤️] great work!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11966#issuecomment-41446437
.

[cvrebert]

@fat So, merge this? It auto-merges cleanly and passes the test suite.

[mdo]

<3

If @fat says looks good, it's him saying go ahead and merge (we talked about it in person awhile back) :).

[lee36656]

NAVER - http://www.naver.com/

[email protected] 님께 보내신 메일 <Re: [bootstrap] Fix #11464 - JS noConflict() mode not working in 3.0.x (#11966)> 이 다음과 같은 이유로 전송 실패했습니다.

---

받는 사람이 회원님의 메일을 수신차단 하였습니다.

---

[zlatanvasovic]

It's nice to see this merged.

2014-05-01 2:46 GMT+02:00 Mark Otto [email protected]:

<3

If @fat https://github.com/fat says looks good, it's him saying go
ahead and merge (we talked about it in person awhile back) :).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11966#issuecomment-41868636
.

Zlatan Vasović - ZDroid