-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Sortable: Improved compatibility with flexbox #1641
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
Conversation
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
I was told to reopen this, and to request the CLA request to be fixed. Apparently multiple users have had this problem. |
Hi @CBenni. Can you please provide some more information about this? What specific problem were you told that other users have had? There are two problems with the current commits:
Beyond the CLA issues, we'd need a unit test showing that this fixes a problem. Are you able to add that? The test should fail without your patch and pass with your patch. Thanks. |
I dont wish to disclose my real name publicly on the internet, as a basic safety precaution. If it is not possible to contribute without doing that, I will close again and use my private fork of the repo in the future. |
That's fine. I've marked your CLA signature as valid. If you can squash your commits and make sure the new commit uses the proper email address, that should get the CLA check to pass. If you're not sure how to do that, you can read through http://contribute.jquery.org/CLA/status/?repo=jquery-ui&sha=1d62f0616cbfb6205d3525adbf81a940e5fc11ae or we can fix this when it's merged. Are you willing to write the unit test? |
Ok, managed to squash and reupload, looks like the CLA worked aswell. Thanks @scottgonzalez I am willing, yet not quite sure how to, write unit tests. This is a one-line change that just modifies behaviour in some fairly specific cases, and has to do with position detection, mouse movement etc. Obviously, a test would be possible for checking if the detection works, I am not sure at how useful that would be, since it only checks a helper function, which is likely to be changed or removed in case of an update. If you could help me/tell me what is to be done, id be happy to help out, albeit I am a newbie when it comes to tests in javascript. |
Added detection for flexbox into _isFloating, which improves the detection of where an item is moved to Ref: #9344
@scottgonzalez If it helps, I have a demo that allows me to reproduce the bug on current versions, and see that the bug is fixed on my fork. I can add that to something somehow, some way. |
We need a reduced test case showing the problem, along with a description of the specific improvement this patch provides. Given the test case that was provided in the ticket, the situation doesn't seem any different with this patch applied. |
Closing due to inactivity. If you're still interested in working on this, please leave another comment addressing our question so we can reopen. |
For all of you stubling across this issue, I found a pretty simple solution to get this working without maintaining a fork: $.widget('custom.flexboxSortable', $.ui.sortable, {
_isFloating: item => {
return (/left|right/).test(item.css('float')) ||
(/inline|table-cell/).test(item.css('display')) ||
(/flex/).test(item.parent().css('display'));
},
});
$('#sortable').flexboxSortable(); Thanks to widget extension |
Added detection for flexbox into _isFloating, which improves the detection of where an item is moved to.
Ref: #9344