Height of droppable area changes after first element dropped

[jbroadway]

trafficstars

Here's a jsfiddle that shows an example of what I mean:

http://jsfiddle.net/6y9w4opk/

If you drag one item from the bottom list into the first, you can drag it anywhere over the <ul> and the item will be moved over correctly (you can see it by the dashed outline of the placeholder).

When you try to drag a second item into the top list, you have to drag it over the previous <li> element or it won't allow you to drop the new item, even though the <ul>'s height hasn't changed.

Here's a video showing what I mean as well:

https://youtu.be/I39e2nlj2HY

It's not that anything breaks, but this has caused confusion for users in two separate test sessions, so we'd like a way to keep the entire <ul> area droppable and have the dropped <li> end up at the end of that list.

Please let me know if there's a workaround or fix I can try. Thanks!

[lukasoppermann]

Thanks for reporting, I guess what happens is, not that the height changes, but rather that we do not detect a drop on a non-empty sortable (<ul>).

When changing this we have to make sure to test that it will only be added to the end of the list if it is not dragged onto another list item.

[JorisM]

Is there an update on this? I encounter the same issue.

[Estigy]

I encountered this, too. What's even worse: I have a setup very much like your last example, "Sortable Copy". When you pull from the source list into an empty spot on the target list, and there is already an item there, the dragged item will be duplicated in the source list, which is meant to not change at all in the first place.

[lukasoppermann]

Hey, there is currently no fix implemented. I would appreciate if one of you could send a PR for this.

What needs to be implemented (I think) is to check onDragEnter if the sortable list is empty.

• If it is empty true the dragged item needs to be added to the sortable
• If it is NOT empty false the nothing changes, so that the item is added after the current item, and not at the end.

[TimE90]

We also encountered this problem. Is there a solution?

[lukasoppermann]

Sadly not. Always happy if you can send a PR with a fix.

[pinta83]

Okay.... this rly pissed me off, so i went digging :)

Here is the solution :)

At around line number 1100 you will find this code:

          // set placeholder height if forcePlaceholderSize option is set
          if (options.forcePlaceholderSize) {
              store(sortableElement).placeholder.style.height = draggingHeight + 'px';
          }

Immediately after that code, paste this:

          if(sortableElement.querySelectorAll(':scope > *' + (options.placeholderClass ? '.' + options.placeholderClass : '')).length < 1){
              element.appendChild(store(sortableElement).placeholder);
		  }

The problem was not the height, but rather a missing placeholder... The code check's if there is a placeholder in the sortable container, and if there is none - just injects one :)

The rest of the code does the job well :)

Hope this helps, it fixed this problem for meh ;)

[erickoledadevrel]

I think an alternative solution is to make a small change in the else case of debouncedDragOverEnter . Replace this:

if (placeholders.indexOf(element) === -1 && sortableElement === element && !_filter(element.children, options.items).length) {

with this:

if (placeholders.indexOf(element) === -1 && sortableElement === element {

That final condition was enforcing that you'd only append the item to the end of the container when there were no existing items (other than the placeholder).

[lukasoppermann]

Hey @erickoledadevrel could you maybe send a PR for this (and do some tests that this does not create any other issue)?

This would be awesome and help to quickly fix this. Thank you. 👍

[erickoledadevrel]

Started looking into it, buy writing the test is much harder than the fix itself 😅. Any pointers to a similar test I could modify would be appreciated.

On Sun, Oct 20, 2019, 8:03 AM Lukas Oppermann [email protected] wrote:

Hey @erickoledadevrel https://github.com/erickoledadevrel could you maybe send a PR for this (and do some tests that this does not create any other issue)?

This would be awesome and help to quickly fix this. Thank you. 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lukasoppermann/html5sortable/issues/172?email_source=notifications&email_token=AAHSBGDWP3TG7EOYU5D43ATQPRCHTA5CNFSM4BM65DYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYISEI#issuecomment-544246033, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHSBGC7UWKXKYEZJW2SPFDQPRCHTANCNFSM4BM65DYA .

[lukasoppermann]

Hey, that is great. Feel free to send an "unfinished" PR if you want.

I don't have a test that really fits, as this part is currently also not covered by at test. However it would be great if you could get it covered.

I am not 100% sure, but maybe it would make sense to extract line 566-576 (https://github.com/lukasoppermann/html5sortable/blob/master/src/html5sortable.ts#L566-L576) to a seperate function, which would be easier to test?

Thank you very much for helping. 👍

[erickoledadevrel]

PR #517 sent!