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

Linked issues

Successfully merging this pull request may close these issues.

3 participants
@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 <borgboyone@users.noreply.github.com>

@mgol mgol added this to the 1.13.1 milestone Nov 7, 2021
@mgol mgol requested a review from fnagel Nov 7, 2021
@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 <borgboyone@users.noreply.github.com>
@mgol mgol force-pushed the sortable-0-height-container-gh-1998 branch from f4d0a0e to 855cf76 Nov 7, 2021
@mgol
Copy link
Member Author

@mgol 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...

Loading

@mgol
Copy link
Member Author

@mgol 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.

Loading

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

@mgol mgol Nov 7, 2021

@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.

Loading

Copy link
Member

@fnagel fnagel Nov 8, 2021

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...

Loading

fnagel
fnagel approved these changes Nov 8, 2021
Copy link
Member

@fnagel fnagel left a comment

+1 by reading

Loading

@borgboyone
Copy link
Contributor

@borgboyone borgboyone commented Nov 8, 2021

@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.

Loading

@mgol
Copy link
Member Author

@mgol 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.

Loading

@mgol mgol merged commit efe3b22 into jquery:main Nov 8, 2021
2 checks passed
Loading
@mgol mgol deleted the sortable-0-height-container-gh-1998 branch Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment