Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

FOR REVIEW: Update to jQuery 2.0.1, LESS 1.3.3 and Bootstrap 2.3.1 #4054

Merged
merged 37 commits into from Jun 4, 2013

Conversation

gruehle
Copy link
Member

@gruehle gruehle commented Jun 2, 2013

This pull requests updates jQuery to 2.0.1, LESS to 1.3.3 and Bootstrap to 2.3.1. This is a major update that has several breaking changes.

Major API changes are listed in the Sprint 26 release notes

Big thanks to @WebsiteDeveloper for the LESS and Bootstrap updates, and @TuckerWhitehouse for the jQuery update.

Remaining tasks:

  • Fix ExtensionManager unit tests (20 failures)
  • Fix FindReplace unit tests (14 failures)
  • Fix Inline Color Editor unit tests (9 failures) (8 fixed, 1 remaining)
  • Fix Url Code Hinting unit tests (2 failures)

WebsiteDeveloper and others added 28 commits May 26, 2013 13:02
explicitly set margin-top to 0
I added this because somehow the relative path of the WebPlatform logo
isn't found.
Conflicts:
	src/htmlContent/about-dialog.html
	src/htmlContent/project-settings-dialog.html
	src/htmlContent/update-dialog.html
	src/styles/brackets.less
Conflicts:
	src/thirdparty/jstree_pre1.0_fix_1/jquery.jstree.js
Required an update to jQuery Mockjax 1.5.2, and quoting in a selector string used
in one of the tests.
@dangoor
Copy link
Contributor

dangoor commented Jun 3, 2013

When I run All of the extension tests, I see a failure in JavaScript Code Hinting that I don't see when I run the JS Code Hinting test on its own. Do you see that as well? I have a feeling that's independent of the work we're tracking here.

As documented in the [jQuery 1.9 upgrade guide](http://jquery.com/upgrade-guide/1.9/#attr-versus-prop-),
the recommended way to update input values is via .val() not .attr().
@dangoor
Copy link
Contributor

dangoor commented Jun 3, 2013

It turns out that the InlineColorEditor failure was actually a case of a jQuery change that required a fix in the code.

@ghost ghost assigned njx Jun 3, 2013
@@ -771,7 +771,7 @@ define(function (require, exports, module) {
* Closes all menus that are open
*/
function closeAll() {
$(".dropdown").removeClass("open");
$(".dropdown").removeClass("open").dropdown("toggle");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for doing both of these things? It seems like this does some redundant work, since .dropdown("toggle") calls clearMenus(), which removes the open class from all dropdown parents. Conversely, since we know for sure we want to remove that class (as opposed to toggling an open dropdown), it seems like just clearing the open class should be good enough (it doesn't look like Dropdown.toggle() does much more than that).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dropdown("toggle") call doesn't seem necessary. I removed it (along with the calls on line 944 and 953) and everything worked fine.

@gruehle
Copy link
Member Author

gruehle commented Jun 4, 2013

@njx - thanks for the review. Changes pushed, and ready for re-review.

} else if ( $.browser.opera ) {
transitionEnd = "oTransitionEnd"
}
transitionEnd = $.support.transition.end;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this change works. It looks like the previous block of code sets $.support.transition to a simple boolean, so it seems like $.support.transition.end would throw an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code set transitionEnd to a string, and so does the new code. (the $.browser property was removed, and the $.support.transition.end property is new)

@njx
Copy link
Member

njx commented Jun 4, 2013

Review complete. The remaining issues I think we should consider addressing before landing the branch are:

  • resolving the $.support.transition issue above
  • If you set brackets.inBrowser to true, the menus don't lay out properly and don't open when you click on them.

There are a few other minor visual issues I'm seeing that I'll go ahead and file with [framework-update] in the subject.

@njx
Copy link
Member

njx commented Jun 4, 2013

Actually, I think we can land this before the in-browser menu work. I'll file a bug on that.

@njx
Copy link
Member

njx commented Jun 4, 2013

I added notes to the bottom of https://github.com/adobe/brackets/wiki/Research:-jQuery-2.0-Upgrade about the changes we made for jQuery 1.9. @dangoor - do you think there's anything else in the 1.9 upgrade guide that's worth specifically warning people about (so they don't have to read all the various obscure things they probably won't care about)?

@njx
Copy link
Member

njx commented Jun 4, 2013

Looks good aside from the filed bugs. Merging.

njx added a commit that referenced this pull request Jun 4, 2013
FOR REVIEW: Update to jQuery 2.0.1, LESS 1.3.3 and Bootstrap 2.3.1
@njx njx merged commit cf4c72a into master Jun 4, 2013
@njx njx deleted the jQueryLESSBootstrap branch June 4, 2013 01:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants