Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sortable: Allow 0-height containers to be sortable as in 1.12.1 #2008

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

mgol
Copy link
Member

@mgol mgol commented Nov 7, 2021

Note that container specific events will not fire when the dragged element
is interacting with zero height containers.

Fixes gh-1998

Co-Authored-By: A. Wells <[email protected]>

@mgol mgol added this to the 1.13.1 milestone Nov 7, 2021
@mgol mgol requested a review from fnagel November 7, 2021 12:42
@mgol mgol self-assigned this Nov 7, 2021
Note that container specific events will not fire when the dragged element
is interacting with zero height containers.

Fixes jquerygh-1998

Co-Authored-By: A. Wells <[email protected]>
@mgol mgol force-pushed the sortable-0-height-container-gh-1998 branch from f4d0a0e to 855cf76 Compare November 7, 2021 12:43
@mgol
Copy link
Member Author

mgol commented Nov 7, 2021

Ouch; I've just discovered the fix only allows the first drag; a second & subsequent ones are still broken just like before. I pushed a test modification that fails due to this. More changes will be needed...

@mgol
Copy link
Member Author

mgol commented Nov 7, 2021

@borgboyone In addition to the changes you described, I also had to remove the if checking for this.innermostContainer !== null in https://github.com/jquery/jquery-ui/blob/1.13.0/ui/widgets/sortable.js#L891-L893 (the statement inside, at L892 is left intact).

This removed all occurrences of this.innermostContainer so I also removed setting the property.

function step1() {
el.find( "li" ).eq( 0 ).simulate( "drag", {
dx: 100,
dy: 3,
Copy link
Member Author

Choose a reason for hiding this comment

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

@fnagel I'm not sure why dy matters here since we're dragging horizontally but it had a visible effect on the test... I'm not sure if it's an actual issue or just a jQuery Simulate quirk but in manual testing it seemed to work fine so I gave up on debugging this.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not really helpful here, but I guess you are right: could be a jQuery Simulate quirk too.
Having some tests should be fine...

Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

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

+1 by reading

@borgboyone
Copy link
Contributor

@mgol - Smart move on removing the other checks for innermostContainer as this point. FYI: As I see it, you'll want to leave L892...just remove L891 and L893.

@borgboyone In addition to the changes you described, I also had to remove the if checking for this.innermostContainer !== null in https://github.com/jquery/jquery-ui/blob/1.13.0/ui/widgets/sortable.js#L891-L893.

This removed all occurrences of this.innermostContainer so I also removed setting the property.

@mgol
Copy link
Member Author

mgol commented Nov 8, 2021

As I see it, you'll want to leave L892...just remove L891 and L893.

Yes, sorry for not being clear, I edited my comment.

@mgol mgol merged commit efe3b22 into jquery:main Nov 8, 2021
@mgol mgol deleted the sortable-0-height-container-gh-1998 branch November 8, 2021 17:21
@mgol
Copy link
Member Author

mgol commented Jan 20, 2022

jQuery UI 1.13.1 including this fix has been released: https://blog.jqueryui.com/2022/01/jquery-ui-1-13-1-released/.

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

Successfully merging this pull request may close these issues.

Sortable Display Grid not working in 1.3
3 participants