Skip to content

Conversation

BlazeCell
Copy link

Sortable's _contactContainers() was modified between version 1.8.24 and version 1.9.1.
In the modification, line 740 was edited from:
"} else if(this.currentContainer != this.containers[innermostIndex]) {"
to:
"} else {"

The modification broke the sorting of floated items.
Sorted items are only able to be placed on the bottom row of sortable containers.

This commit reverts line 740 back to the 1.8.24 version that works.

Sortable's _contactContainers() was modified between version 1.8.24 and version 1.9.1.
In the modification, line 740 was edited from:
"} else if(this.currentContainer != this.containers[innermostIndex]) {"
to:
"} else {"

The modification broke the sorting of floated items.
Sorted items are only able to be placed on the bottom row of sortable containers.

This commit reverts line 740 back to the 1.8.24 version that works.
@@ -737,7 +737,7 @@ $.widget("ui.sortable", $.ui.mouse, {
if(this.containers.length === 1) {
this.containers[innermostIndex]._trigger("over", event, this._uiHash(this));
this.containers[innermostIndex].containerCache.over = 1;
} else {
} else if(this.currentContainer != this.containers[innermostIndex]) {
Copy link
Author

Choose a reason for hiding this comment

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

Reverted line back to 1.8.24 version.

@mikesherov
Copy link
Member

Hi @BlazeCell, can you add unit tests to support your change?

@mikesherov
Copy link
Member

Nevermind, this causes a reversion in 3 bugs. Sorry that we didn't add tests!

@mikesherov
Copy link
Member

@BlazeCell, we're going to close this because it regresses elsewhere. I appreciate the contribution, and if you can come up with a different fix that doesn't break the other fixes, we'll gladly accept it. Please let me know if you have further questions.

@mikesherov mikesherov closed this Nov 9, 2012
@joshuaballoch
Copy link

@mikesherov is there any chance that the jQuery ui team is looking into a solution? I opened a new ticket on this issue, as it still exists, and my ticket was closed, referencing the issue that is linked to this pull request. Seeing as this pull request wasn't able to provide a good solution to this problem, the bug still exists.

Here is the new ticket I opened to suggest fixing this bug: http://bugs.jqueryui.com/ticket/9070#comment:2

@scottgonzalez
Copy link
Member

@zhizhangchen Would you be interested in looking at http://bugs.jqueryui.com/ticket/8792 (and related http://bugs.jqueryui.com/ticket/9041)?

@zhizhangchen
Copy link
Contributor

OK, I'll look into that.

@scottgonzalez
Copy link
Member

Thanks.

@zhizhangchen
Copy link
Contributor

I've found a solution for this problem. Which branch should the fix be based on, 1.9.1 or master?

@tjvantoll
Copy link
Member

@zhizhangchen master.

@zhizhangchen
Copy link
Contributor

I've sent a pull request to fix these problems: #916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants