Bug: sort method doesn't work if there are other children in the sortable parent

[pr3tori4n]

Problem:

If you create a sortable on a container, and use the draggable selector to identify the sortable elements, but there are other non-sortable sibling elements in that container, the sort method fails. An example would be if a header was the first child of the sortable container.

The problem is this line. This is getting the child at the index of the loop, but it's considering all child elements. It needs to first sort out those that don't match the draggable selector. https://github.com/SortableJS/Sortable/blob/master/src/Sortable.js#L1571

JSBin/JSFiddle demonstrating the problem:

https://jsfiddle.net/pr3tori4n/k17dhqo8/

[jamigibbs]

I'm curious why you would need a header within the list to begin with? Couldn't that just go outside of it?

  <header>header</header>
  <div id="sortTrue" class="list-group">
    <div class="list-group-item">foo</div>
    <div class="list-group-item">bar</div>
    <div class="list-group-item">baz</div>
  </div>

[tusharsrivastava]

Hi @jamigibbs There is an example use-case where we'll need nested block. For example,

<!-- Main Bucket -->
<div data-bucket="list-bucket">
    <div class="bucket">
        <header>Bucket Name</header>
        <div class="bucket-items">
            <div class="item">...</div>
            <div class="item">...</div>
            <div class="item">...</div>
        </div>
    </div>
    <!-- More buckets like above -->
</div>
<!-- Shortlisted Bucket -->
<div data-bucket="shortlist-bucket">
    <div class="bucket">
        <header>Bucket Name</header>
        <div class="bucket-items"></div>
</div>

Now for the above structure, I only want the .item entries to be draggable between the buckets and I will configure it like below:

var mainBucket = document.querySelector('[data-bucket="list-bucket"]');
var shortlistedBucket = document.querySelector('[data-bucket="shortlist-bucket"]');

var dndMainBucket = new Sortable(mainBucket, {
    group: 'buckets',
    multiDrag: true,
    draggable: '.item'
});
var dndShortlistedBucket = new Sortable(shortlistedBucket, {
    group: 'buckets',
    multiDrag: true,
    draggable: '.item'
});

But with existing structure, the sortable won't work.

Any suggestions?

[jamigibbs]

@tusharsrivastava You would just need to register the bucket-items lists with Sortable; not just the data-bucket attribute:

var mainBucket = document.querySelector('[data-bucket="list-bucket"] .bucket-items');
var shortlistedBucket = document.querySelector('[data-bucket="shortlist-bucket"] .bucket-items');

Example: https://jsbin.com/wakunip/30/edit?html,js,output

[tusharsrivastava]

@jamigibbs Actually, I've done something similar. Let's see the issue here:

1. I've a simple 2 column layout
2. First column list bucket items with bucket name as header, Second column is the empty space to drop content from bucket to.
3. Each bucket (bucket name) is its own group to make sure we can't drag and drop between buckets.
4. Now, the drop region (second column) either need a way to support drop from multiple groups, or multiple rows need to be defined in second column with right group names to facilitate drop on 2nd column.
5. Further, if multiple rows are defined in second column, it become difficult for someone to drop an item from 1st column to second as they need to align their drop on correct row on 2nd column.

Here's a demo video: video link

I'm sure if the container (column 1 and column 2) could've been made a drop-zone such that I could drop it anywhere on 2nd column and it will take its position according to bucket name it is in. Also, I only needed to define single group, for 1st and 2nd column.

Any idea?

[h0lg]

I encountered this issue with a different list structure as well. In my example (a trip itinerary), there is a list with headers, the first of which is not draggable (, because the headers represent the stop status and sorting a stop before the visited header would result in an undefined status).

• visited (not draggable)
• stop 1
• stop 2
• planned
• stop 3
• stop 4
• optional
• stop 5
• stop 6

The other headers are draggable to "progress" items into another group while maintaining their order. I.e. if you had planned stop 3 and 4 for today and you did them in that order, you could drag the "planned" header to the bottom of stop 4 and update both stops in one move instead of having to drag stop 3 into the visited list and then do the same for stop 4.

If I try to cancel an itinerary update during which the traveler can re-order the stops on it, the current implementation of the sort() method doesn't restore the original order (which I remember on initialization using toArray()). I've stepped through it in the debugger and have commented my findings:

    sort: function sort(order) {
      var items = {},
          rootEl = this.el;

      //this.toArray() ignores first header, because it's not draggable
      this.toArray().forEach(function (id, i) {
        var el = rootEl.children[i]; //but index i doesn't ignore non-draggables and will point to it

        if (closest(el, this.options.draggable, rootEl, false)) { //first element is not draggable; first id never set on items
          items[id] = el; //on consecutive runs, the element is stored by the id of its next sibling; 2 => stop#1, 3 => stop#2
        }
      }, this);
      order.forEach(function (id) {
        if (items[id]) {
          rootEl.removeChild(items[id]);
          rootEl.appendChild(items[id]);
        }
      });
    },

Obviously, moving the first header out of and above the list as @jamigibbs proposed above is the work-around in my case as well and I will use it.

But still, this raises the question of whether I understood the intended use of the "draggable" option correctly. Because I don't see how the current implementation of sort() could treat non-draggable items in between the draggable ones correctly, if I wanted to do that. I see two distinct problems:

1. toArray() as used in the above implementation ignores non-draggable items while the expression var el = rootEl.children[i]; (with i being the index of the draggable item id) doesn't.
2. the order parameter is treated like an array of ids of draggable items and doesn't expect or handle non-draggable items that could be among the draggable ones. That is, the forEach handler only re-orders elements whose id is found in items, which was previously only filled with draggable elements.

Let me know if I didn't manage to make myself clear - I realize this may be hard to understand.

Oh, and thank you for your continuous work on this great library <3

[h0lg]

On second thought, changing the implementation of toArray to return all element ids instead of only those of the draggable ones could solve above issues.

    toArray: function toArray() {
      var order = [],
          el,
          children = this.el.children,
          i = 0,
          n = children.length,
          options = this.options;

      for (; i < n; i++) {
        el = children[i];

        if (closest(el, options.draggable, this.el, false)) { //this condition would have to be removed
          order.push(el.getAttribute(options.dataIdAttr) || _generateId(el)); //so that all element ids would be appended here
        }
      }

      return order;
    }

Obviously, that changes the return value of toArray() and thus the external API, which would have to be communicated, unless toArray() gets a parameter to include non-draggable items that is false by default, which would only change its behavior if explicitly called with true.

    toArray: function toArray(includeNonDraggables) {
      var order = [],
          el,
          children = this.el.children,
          i = 0,
          n = children.length,
          options = this.options;

      for (; i < n; i++) {
        el = children[i];

        //include non-draggables here if toArray() is explicitly called with true
        if (includeNonDraggables || closest(el, options.draggable, this.el, false)) {
          order.push(el.getAttribute(options.dataIdAttr) || _generateId(el));
        }
      }

      return order;
    }

With this, the implementation of sort() could be changed to call toArray(true) which would allow sort() to handle non-draggable items just fine - if I understand it correctly. This also means a de facto change in the external API of sort() in that it would now expect the ordered array of all ids instead of just the draggable ones. But as it doesn't currently handle lists including non-draggable items correctly, I don't think that's a big issue.

BTW, I've looked at the internal use of toArray() and sort() seems to be the only one.

Let me know what you think. Cheers!

[ogizanagi]

I'm encountering the same issue as of today, with non-draggable items.

When every items in the list is draggable, the sort method behaves properly and I can restore the previous order on click on a cancel button. But as soon as there is one item non-draggable (some items can be fixed to the top), the cancel button changes the order in an inappropriate way.

The above investigation looks fair to me. So what about introducing a boolean argument in toArray() and always use it to true internally to consider the non-draggable elements properly?

[botandrose]

I also ran into this today. Thankful for this thread for helping figure out what the heck was happening! Agree that it would be nice to have this work by default, or throw a warning in the console when there are non-sortable children as siblings, at the very least..