Resizable: Refactored resize handler. Fixed #5545, #5817, #7605. #814

Closed
wants to merge 5 commits into
from

Projects

None yet

2 participants

@eromba
Contributor
eromba commented Nov 8, 2012

(Follow-up to jquery/jquery-ui#694)

This pull request includes a refactoring of Resizable's resize handler that fixes 3 reported issues. The fixes build upon one another, so I am including them together in one request. I've included appropriate test cases for each issue, as well.

Resizable now modifies only the CSS properties that are actually necessary to perform the resize (instead of always setting top, left, width, and height regardless; see #7605). This makes the plugin behave more predictably in dynamic layouts.

Furthermore, the plugin's resize event is now altogether more useful:

  • The event is triggered only when the element's size is modified (as opposed to every mousemove; see #5545)
  • The ui callback parameter now correctly reports the element's current size (see #5817)
@mikesherov mikesherov commented on an outdated diff Nov 9, 2012
tests/unit/resizable/resizable_events.js
+ }
+ });
+
+ TestHelpers.resizable.drag(handle, 50, 50);
+
+ equal(count, 1, "start callback should happen exactly once");
+
+});
+
+test("resize", function() {
+
+ expect(9);
+
+ var count = 0,
+ handle = '.ui-resizable-se';
+ el = $("#resizable1").resizable({
mikesherov
mikesherov Nov 9, 2012 Member

el is leaking globally here. That should be avoided. Have you run grunt on the code here?

@mikesherov mikesherov commented on an outdated diff Nov 9, 2012
tests/unit/resizable/resizable_events.js
+test("resize", function() {
+
+ expect(9);
+
+ var count = 0,
+ handle = '.ui-resizable-se';
+ el = $("#resizable1").resizable({
+ handles: 'all',
+ resize: function(event, ui) {
+ if (count === 0) {
+ equal( ui.size.width, 101, "compare width" );
+ equal( ui.size.height, 101, "compare height" );
+ equal( ui.originalSize.width, 100, "compare original width" );
+ equal( ui.originalSize.height, 100, "compare original height" );
+ }
+ else {
mikesherov
mikesherov Nov 9, 2012 Member

} else { instead of a new line.

@mikesherov mikesherov commented on the diff Nov 9, 2012
ui/jquery.ui.resizable.js
@@ -286,8 +286,11 @@ $.widget("ui.resizable", $.ui.mouse, {
_mouseDrag: function(event) {
//Increase performance, avoid regex
- var el = this.helper,
- smp = this.originalMousePosition, a = this.axis;
+ var el = this.helper, props = {},
+ smp = this.originalMousePosition, a = this.axis,
+ prevTop = this.position.top, prevLeft = this.position.left,
mikesherov
mikesherov Nov 9, 2012 Member

Perhaps create a map here instead of four variables to track the values?

eromba
eromba Nov 9, 2012 Contributor

I don't believe there is any real file-size or clarity benefit from using a map. Besides, doing so would add the (negligible?) overhead of creating a new object. Since this code is run on every mouse move during a resize operation, we might as well error on the side of performance, right?

mikesherov
mikesherov Nov 9, 2012 Member

Right, I was only suggesting a map to use for the loop, which I went back on. So this is fine. :)

@mikesherov mikesherov commented on the diff Nov 9, 2012
ui/jquery.ui.resizable.js
// plugins callbacks need to be called first
this._propagate("resize", event);
- el.css({
- top: this.position.top + "px", left: this.position.left + "px",
- width: this.size.width + "px", height: this.size.height + "px"
- });
+ if (this.position.top !== prevTop) {
mikesherov
mikesherov Nov 9, 2012 Member

Maybe a loop here in conjunction with a map from above rather than 4 conditionals?

mikesherov
mikesherov Nov 9, 2012 Member

Nevermind, didn't see that one was position and one was size

@mikesherov mikesherov commented on an outdated diff Nov 9, 2012
tests/unit/resizable/resizable_events.js
@@ -5,8 +5,161 @@
module("resizable: events");
-// this is here to make JSHint pass "unused", and we don't want to
-// remove the parameter for when we finally implement
-$.noop();
+test("start", function() {
+
+ expect(5);
+
+ var count = 0,
+ handle = '.ui-resizable-se';
mikesherov
mikesherov Nov 9, 2012 Member

I know the existing tests don't currently reflect this, but the standard is actually double quotes for strings.

Member

Thanks for redoing these pulls, especially the tests. The interactions are undergoing a rewrite and even if the code here gets rewritten, the tests will be invaluable in preventing regression! Thanks again!

Contributor
eromba commented Nov 9, 2012

I believe this commit addresses the stylistic points you mentioned. Thanks for the thorough review!

Member

@eromba, thanks again for making the changes! 2 last things. You mentioned on the bug tracker this fixes 4554 and 7193 as well. Can you add two tests for those as well? And lastly, did you run the tests in all of our supported browsers?

Member

Thanks again for everything here. Can you also edit the description when you add those tests to mention they also fix http://bugs.jqueryui.com/ticket/4554 and http://bugs.jqueryui.com/ticket/7193 ?

@mikesherov mikesherov commented on the diff Nov 13, 2012
ui/jquery.ui.resizable.js
if (!this._helper && this._proportionallyResizeElements.length)
this._proportionallyResize();
- this._updateCache(data);
mikesherov
mikesherov Nov 13, 2012 Member

why did this get moved up to before the internal propagate? I'm just trying to understand how that effects the internal state...

Member

@eromba, I'm actually going to land this. If at some point you want to write unit tests showing those other two bugs are fixed by this, that would be super awesome, but it's not required! Thanks again!

Member

Thanks! landed in 3974b55

@mikesherov mikesherov closed this Nov 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment