Resizable: modified resizable containment plugin to restrict resizing within container. Fixes #7018 Resizable: resizing can move objects #1122
Conversation
|
The approach seems fine to me. I have some minor nits, which I'll report now just to avoid saying them later:
|
|
@mikesherov added unit test cases and your suggested changes. |
| @@ -123,6 +123,27 @@ test("aspectRatio: 'preserve' (ne)", function() { | |||
| equal( target.height(), 70, "compare minHeight"); | |||
| }); | |||
|
|
|||
| test( "aspectRatio: ticket-7018", function() { | |||
There was a problem hiding this comment.
We should avoid ticket references in unit tests. Describe what the test is about instead, like all other tests do.
There was a problem hiding this comment.
Yes. The test name should be descriptive of the test. Ticket references should be in comments.
There was a problem hiding this comment.
Is this a good name aspectRatio:position ? i am very bad in naming variables
There was a problem hiding this comment.
I was under the impression that test names should reference ticket number along with good description. I thought we were just getting rid of *_tickets.js. It's helpful to see the ticket number in the runner output.
There was a problem hiding this comment.
I'm ok with that, as long as the test name is descriptive. I just don't think the fact that a ticket existed is actually important in the output.
There was a problem hiding this comment.
@scottgonzalez I'm fine either way, but I do personally think the fact that a ticket existed is important, I always look back at fiddles.
There was a problem hiding this comment.
removed ticket ref from test output, but added #ticket as comment in code.
|
@dekajp, thanks again for all your hard work! I did a review and once you make those minor changes, I can land this! |
|
@mikesherov thank you !! whats your preference - do you want me to push new changes in separate commit or you want me to update original commit ? |
|
@dekajp single updated commit is preferred. |
… within container. Fixes #7018 Resizable: resizing can move objects
|
@mikesherov seems http://bugs.jqueryui.com/ticket/9107 is related to this . here is the fixed fiddle -http://jsfiddle.net/Mm2Ed/3/ ( minor change - do you want me to add this change in this commit or i can submit a new PR based on this . |
|
Add it to this, but edit the commit message. Thanks again for all your hard work! |
|
I am trying to add a test case - example in fiddle (http://jsfiddle.net/c2Rux/16/) it seems that i can not simulate this the last drag |
|
ah !! i think i got the problem , i created some room for mouse move by making the container |
… within container. Fixes #7018 Resizable: resizing can move objects and Fixes #9107 Resizable: aspectRatio and containment not handled correctly
Fixes #9107 Resizable: aspectRatio and containment not handled correctly
Fixes #7018 Resizable: resizing can move objects
Just want to check if this approach is ok ? for fiddle link - http://jsfiddle.net/dekajp/c2Rux/14/
adding
prevSizeandprevPositionin the plugin parameter object. so that if resize breaks then it can be restored.this PR is not complete - does not include unit testing. ( Fixes #7018 ). also i found some minor code style issues , so i fixed them.