Resizable: Remove bad workaround for draggable+resizable #1210
Conversation
| minHeight: 13, | ||
| handles: "s" | ||
| }); | ||
| $(".draggable:last").addClass("absolute"); |
|
Other than the minor spacing comments I think this looks good. I see no reason for that workaround to remain in place. |
|
Is there a unit test that can cover this change? |
|
I've fixed the spacing issues. I've looked through the existing unit tests for resizable. There's nothing testing draggable+resizable, and I couldn't find a test for the absolute positioning. I guess we'd need a test that verifies the position of the handles after a resize. None of the existing tests seem to do that. Does that make sense? |
|
Right, none of the tests do that (resizable's suite sucks, as do the other interactions besides draggable), so let's add one :-) |
|
Just updated this, extending the one unit test that actually used absolute positioning. I added a check for the position of the handle after resizing. After I added that test, I checked what happens if I remove the workaround altogether and the test still passed. I then checked my visual test page again and its also working fine. I can't reproduce the issue I thought I saw earlier, therefore removed the workaround completely. Ran the updated tests through our 12 supported browsers through browserstack-runner, all passed. Doesn't mean much given the resizable testsuite, but at least there's no regression here. Thoughts? |
|
Considering that no workaround is necessary at all, do you think we need the new visual test page? |
|
I still think the visual test page is useful, since we have nothing else that tests the combination of draggable and resizable. |
|
I guess this is good to land then. |
This adds a compound test page for draggable+resizable, which had no coverage before. Using that page shows that there is no way to reproduce the behaviour described in the original ticket that caused this workaround, since its not possible to resize an element beyond the window boundaries. Therefore removing the workaround, which is 6+ years old and has no test coverage, seems like the sanest approach. Fixes #6939 Closes gh-1210
This adds a compound test page for draggable+resizable, which had no coverage
before. Using that page shows that there is no way to reproduce the behaviour
described in the original ticket that caused this workaround, since its not
possible to resize an element beyond the window boundaries. Therefore removing
the workaround, which is 6+ years old and has no test coverage, seems like the
sanest approach.
Fixes #6939