Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

CBenni
Copy link

@CBenni CBenni commented Nov 1, 2015

Added detection for flexbox into _isFloating, which improves the detection of where an item is moved to.

Ref: #9344

@jquerybot
Copy link

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.

@CBenni
Copy link
Author

CBenni commented Nov 3, 2015

I was told to reopen this, and to request the CLA request to be fixed. Apparently multiple users have had this problem.

@CBenni CBenni reopened this Nov 3, 2015
@scottgonzalez
Copy link
Member

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:

  • One commit uses a noreply email address, which we don't accept because they're effectively the same as not having an email address.
  • We request your full name, not a username or nick, but the CLA and commits have the name CBenni.

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.

@CBenni
Copy link
Author

CBenni commented Nov 3, 2015

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.
As for the e-mail, I do not know how to change the old commit to have the same e-mail, the second commit was changed using an --amend.

@scottgonzalez
Copy link
Member

I dont wish to disclose my real name publicly on the internet, as a basic safety precaution.

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?

@CBenni
Copy link
Author

CBenni commented Nov 3, 2015

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
@CBenni
Copy link
Author

CBenni commented Nov 5, 2015

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

@scottgonzalez
Copy link
Member

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.

@scottgonzalez
Copy link
Member

Closing due to inactivity. If you're still interested in working on this, please leave another comment addressing our question so we can reopen.

@Basster
Copy link

Basster commented Oct 4, 2019

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

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.

4 participants