-
Notifications
You must be signed in to change notification settings - Fork 7.6k
FOR REVIEW: Update to jQuery 2.0.1, LESS 1.3.3 and Bootstrap 2.3.1 #4054
Changes from 33 commits
ff8d55c
54394b0
6c23c49
de6c29d
b276e04
ceb6297
c21ab02
b34eb06
ceca9bc
ad63c37
c58b5e6
026f9dd
9c6a57e
43d5435
180ad2a
93b629d
1978f85
1cb9550
d8b9421
fd77d76
3da4dcb
c04cf47
ec69402
2e777df
72f652f
6cc7b77
bb5a7f1
79f799a
305a196
c1bdc00
30e3c12
435c3e9
0f747ca
81eff5f
d454d87
c3acbd6
26e12d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,12 +121,12 @@ define(function (require, exports, module) { | |
this.$msgArea.hide(); | ||
this.$inputArea.show(); | ||
this.$okButton | ||
.attr("disabled", "disabled") | ||
.prop("disabled", true) | ||
.text(Strings.INSTALL); | ||
break; | ||
|
||
case STATE_VALID_URL: | ||
this.$okButton.removeAttr("disabled"); | ||
this.$okButton.prop("disabled", false); | ||
break; | ||
|
||
case STATE_INSTALLING: | ||
|
@@ -136,7 +136,7 @@ define(function (require, exports, module) { | |
this.$msg.text(StringUtils.format(Strings.INSTALLING_FROM, url)) | ||
.append("<span class='spinner spin'/>"); | ||
this.$msgArea.show(); | ||
this.$okButton.attr("disabled", "disabled"); | ||
this.$okButton.prop("disabled", true); | ||
this._installer.install(url) | ||
.done(function () { | ||
self._enterState(STATE_INSTALLED); | ||
|
@@ -156,7 +156,7 @@ define(function (require, exports, module) { | |
case STATE_CANCELING_INSTALL: | ||
// This should call back the STATE_INSTALLING fail() handler above, unless it's too late to cancel | ||
// in which case we'll still jump to STATE_INSTALLED after this | ||
this.$cancelButton.attr("disabled", "disabled"); | ||
this.$cancelButton.prop("disabled", true); | ||
this.$msg.text(Strings.CANCELING_INSTALL); | ||
this._installer.cancel(); | ||
window.setTimeout(function () { | ||
|
@@ -292,7 +292,7 @@ define(function (require, exports, module) { | |
this.$dlg = $(".install-extension-dialog.instance"); | ||
this.$url = this.$dlg.find(".url").focus(); | ||
this.$okButton = this.$dlg.find(".dialog-button[data-button-id='ok']"); | ||
this.$cancelButton = this.$dlg.find(".dialog-button[data-button-id='cancel']"); | ||
this.$cancelButton = this.$dlg.find(".dialog-button[data-dismiss='modal']"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why this change is necessary since that button still has |
||
this.$inputArea = this.$dlg.find(".input-field"); | ||
this.$msgArea = this.$dlg.find(".message-field"); | ||
this.$msg = this.$msgArea.find(".message"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,7 +157,7 @@ define(function (require, exports, module) { | |
hueColor = "hsl(" + this._hsv.h + ", 100%, 50%)"; | ||
|
||
this._updateColorTypeRadioButtons(colorObject.format); | ||
this.$colorValue.attr("value", colorValue); | ||
this.$colorValue.val(colorValue); | ||
this.$currentColor.css("background-color", colorValue); | ||
this.$selection.css("background-color", hueColor); | ||
this.$hueBase.css("background-color", hueColor); | ||
|
@@ -597,7 +597,7 @@ define(function (require, exports, module) { | |
case KeyEvent.DOM_VK_LEFT: | ||
case KeyEvent.DOM_VK_RIGHT: | ||
step = event.shiftKey ? step * STEP_MULTIPLIER : step; | ||
xOffset = Number($.trim(this.$selectionBase.css("left").replace("%", ""))); | ||
xOffset = Number($.trim(this.$selectionBase[0].style.left.replace("%", ""))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dangoor - Why was this change necessary? I didn't see anything in the migration guides about it. Is it because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this isn't in the migration guides, but was a change in jQuery 1.8. The css() function always returns pixel values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I ran into this issue in my original pull for the jQuery 2.0 upgrade, it seems that jQuery makes no guarantee about the units it will return (in my findings, the left value returned in pixels, but the bottom value returned the expected [as authored] percentage). Not sure if it has since been updated to always return pixels? |
||
adjustedOffset = (event.keyCode === KeyEvent.DOM_VK_LEFT) ? (xOffset - step) : (xOffset + step); | ||
xOffset = Math.min(100, Math.max(0, adjustedOffset)); | ||
hsv.s = xOffset / 100; | ||
|
@@ -606,7 +606,7 @@ define(function (require, exports, module) { | |
case KeyEvent.DOM_VK_DOWN: | ||
case KeyEvent.DOM_VK_UP: | ||
step = event.shiftKey ? step * STEP_MULTIPLIER : step; | ||
yOffset = Number($.trim(this.$selectionBase.css("bottom").replace("%", ""))); | ||
yOffset = Number($.trim(this.$selectionBase[0].style.bottom.replace("%", ""))); | ||
adjustedOffset = (event.keyCode === KeyEvent.DOM_VK_DOWN) ? (yOffset - step) : (yOffset + step); | ||
yOffset = Math.min(100, Math.max(0, adjustedOffset)); | ||
hsv.v = yOffset / 100; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -399,7 +399,7 @@ define(function (require, exports, module) { | |
runs(function () { | ||
makeUI(colorStr); | ||
expect(colorEditor.getColor().toString()).toBe(colorStr); | ||
expect(colorEditor.$colorValue.attr("value")).toBe(colorStr); | ||
expect(colorEditor.$colorValue.val()).toBe(colorStr); | ||
expect(tinycolor.equals(colorEditor.$currentColor.css("background-color"), colorStr)).toBe(true); | ||
|
||
// Not sure why the tolerances need to be larger for these. | ||
|
@@ -415,7 +415,7 @@ define(function (require, exports, module) { | |
runs(function () { | ||
checkPercentageNear(colorEditor.$hueSelector.css("bottom"), 25); | ||
checkPercentageNear(colorEditor.$opacitySelector.css("bottom"), 50); | ||
checkPercentageNear(colorEditor.$selectionBase.css("left"), 74); | ||
checkPercentageNear(colorEditor.$selectionBase[0].style.left, 74); | ||
checkPercentageNear(colorEditor.$selectionBase.css("bottom"), 47); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dangoor - related to the previous question :), why do we need to do this for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That test wasn't failing... But yes, it should use the same syntax for consistency. Fixed. |
||
}); | ||
}); | ||
|
@@ -428,7 +428,7 @@ define(function (require, exports, module) { | |
makeUI("#0a0a0a"); | ||
colorEditor.setColorFromString(colorStr); | ||
expect(colorEditor.getColor().toString()).toBe(colorStr); | ||
expect(colorEditor.$colorValue.attr("value")).toBe(colorStr); | ||
expect(colorEditor.$colorValue.val()).toBe(colorStr); | ||
expect(tinycolor.equals(colorEditor.$currentColor.css("background-color"), colorStr)).toBe(true); | ||
checkNear(tinycolor(colorEditor.$selection.css("background-color")).toHsv().h, tinycolor(colorStr).toHsv().h); | ||
checkNear(tinycolor(colorEditor.$hueBase.css("background-color")).toHsv().h, tinycolor(colorStr).toHsv().h); | ||
|
@@ -441,7 +441,7 @@ define(function (require, exports, module) { | |
runs(function () { | ||
checkPercentageNear(colorEditor.$hueSelector.css("bottom"), 25); | ||
checkPercentageNear(colorEditor.$opacitySelector.css("bottom"), 50); | ||
checkPercentageNear(colorEditor.$selectionBase.css("left"), 74); | ||
checkPercentageNear(colorEditor.$selectionBase[0].style.left, 74); | ||
checkPercentageNear(colorEditor.$selectionBase.css("bottom"), 47); | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,6 @@ <h1 class="dialog-title">{{INSTALL_EXTENSION_TITLE}}</h1> | |
<div class="modal-footer"> | ||
<button class="btn left browse-extensions">{{BROWSE_EXTENSIONS}}</button> | ||
<button class="dialog-button btn primary" data-button-id="ok" disabled>{{INSTALL}}</button> | ||
<button class="dialog-button btn" data-button-id="cancel">{{CANCEL}}</button> | ||
<button class="dialog-button btn" data-button-id="cancel" data-dismiss="modal">{{CANCEL}}</button> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear why we're adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (My belief is that we shouldn't be adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. According to the documentation, |
||
</div> | ||
</div> |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// | ||
// Accordion | ||
// -------------------------------------------------- | ||
|
||
|
||
// Parent container | ||
.accordion { | ||
margin-bottom: @baseLineHeight; | ||
} | ||
|
||
// Group == heading + body | ||
.accordion-group { | ||
margin-bottom: 2px; | ||
border: 1px solid #e5e5e5; | ||
.border-radius(@baseBorderRadius); | ||
} | ||
.accordion-heading { | ||
border-bottom: 0; | ||
} | ||
.accordion-heading .accordion-toggle { | ||
display: block; | ||
padding: 8px 15px; | ||
} | ||
|
||
// General toggle styles | ||
.accordion-toggle { | ||
cursor: pointer; | ||
} | ||
|
||
// Inner needs the styles because you can't animate properly with any styles on the element | ||
.accordion-inner { | ||
padding: 9px 15px; | ||
border-top: 1px solid #e5e5e5; | ||
} |
There was a problem hiding this comment.
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")
callsclearMenus()
, which removes theopen
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 theopen
class should be good enough (it doesn't look likeDropdown.toggle()
does much more than that).There was a problem hiding this comment.
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.