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

Remove on spill #1465

Closed
JivanRoquet opened this issue Mar 9, 2019 · 15 comments
Closed

Remove on spill #1465

JivanRoquet opened this issue Mar 9, 2019 · 15 comments
Labels
Projects

Comments

@JivanRoquet
Copy link

JivanRoquet commented Mar 9, 2019

As I browsed through the history of issues, it seems that a lot of people are facing the same problem: there is no standard and built-in way to replicate Dragula's extremely convenient feature of removeOnSpill.

The idea is that if you have one or more containers belonging to the same group, then if you drop any element out of these containers, then the element would be simply deleted from where it was.

I understand that the name "sortable" refers to a library for sorting, but honestly it's been long since this library has started to do a lot of different things beyond the pure "sorting" scope (for instance, exchanging elements from one container to another: it's not "sorting" per se anymore).

It seems to be possible to implement that behaviour currently, but:

  • it seems hacky (especially "removing the element after onAdd" — not the definition of clean)
  • this is very cumbersome (an option would make that into a one-liner)
  • there is no standard way
  • it is a complex (and more cumbersome even, in a non-reproducible manner) thing to get right when you have a non-trivial setup (like, one immutable source container and several other normal containers between which elements can be exchanged, or imported from the source)
  • it it hard to integrate with real-world environments and in particular state management systems (React/Redux, Vue/Vuex...) as it's mostly made of tricks that consists of playing with the DOM (a big no-no in most modern frameworks)

One dream solution for this would be something similar to:

// normal container
var sortable = new Sortable(el, {
  group: { name: 'myGroup' },
  removeOnSpill: true // here "spill" means dropping at any place which is not part of the "group"
})

// "copy" container
var sortable = new Sortable(el, {
  group: {
    name: 'myGroup',
    pull: 'clone',
    put: false
  },
  removeOnDrop: true // remove any element dropped here from another container belonging to the same group
})

Another possible implementation could be:

// normal container
var sortable = new Sortable(el, {
  group: { name: 'myGroup' },
  keepOnly: [el1, el2, el3] // the item is removed if dropped anywhere out of these DOM elements
})

Overall, SortableJS is massively superior to Dragula in terms of options and smoothness of animations and movement. This really is an amazing piece of work, and this lack of "remove on spill" option is the only caveat which SortableJS has compared to Dragula. Given the number of people asking for this, it seems to be a massive one unfortunately.

Happy to help if needed, however I'm not really familiar with the library's code yet, so might not be the best person yet to implement a relatively complex feature like this one.

@owen-m1
Copy link
Member

owen-m1 commented Mar 9, 2019

@JivanRoquet This option is entirely possible, and I too would like for it to be implemented. Unfortunately, there is a lot to do first that I have already started working on. This can be in the next minor version (see the next-version branch), but I am in the middle of completely revamping and improving the animation system, which is taking a while and it will still be a while until it is complete. If you want to make a PR on that branch it would help, the feature doesn't seem that complex at first glance. Really you would just check in _onDrop if the target(?) is of a sortable in it's group, and if not remove dragEl from the DOM. But I can implement it myself too; it probably wouldn't make much of a difference in terms of time until it is released.

Thank you for creating this issue.

@smg99
Copy link

smg99 commented Mar 28, 2019

@JivanRoquet Did you fixed this issue ? I tried to identify if the item is dropped out of list, however the onEnd doesn't seems to provide the information needed to identify the correct target. @owen-m1 Any hint?

@owen-m1
Copy link
Member

owen-m1 commented Mar 28, 2019

@sumitg-3767 I would set a dragend, mouseup, and touchend event on the element being dragged. You can set the events in onStart and remove them in onEnd. For the mouse and touch events, check if the target is not a Sortable element or is not contained in a Sortable element. For a dragend event, it is a bit more complicated, because it does not give you the actual target it is dropped on. You will need to set a drop event on the Sortable(s), which when fired will set a global variable dropped to true. Then, in the dragend event, if dropped is false you must remove it. dropped must be set to false in onStart and best to initialize it as false as well. Something like this:

var dropped = false, // is drop valid?
	item; // dragged item

var dragEnd = function(evt) {
	if (!dropped) {
		item.parentNode.removeChild(item); // Remove dragged item from DOM
	}
};

new Sortable(el, {
	onStart: function(evt) {
		dropped = false;
		item = evt.item;
		item.addEventListener('dragend', dragEnd);
		// and set mouse & touch events
	},

	onEnd: function(evt) {
		item.removeEventListener('dragend', dragEnd);
		// and remove mouse & touch events
	}
});

el.addEventListener('drop', function() {
	dropped = true;
});

@smg99
Copy link

smg99 commented Mar 30, 2019

It worked indeed ! Except that onEnd of Sortable executes first(which would not allow dragEnd to be executed) before dragEnd, so removing event listener should be part of dragEnd

var dropped = false, // is drop valid?
item; // dragged item

var dragEnd = function(evt) {
if (!dropped) {
item.removeEventListener('dragend', dragEnd);
item.parentNode.removeChild(item); // Remove dragged item from DOM
}
};

new Sortable(el, {
onStart: function(evt) {
dropped = false;
item = evt.item;
item.addEventListener('dragend', dragEnd);
// and set mouse & touch events
},

onEnd: function(evt) {
	item.removeEventListener('dragend', dragEnd);
	// and remove mouse & touch events
}

});

el.addEventListener('drop', function() {
dropped = true;
});

@owen-m1
Copy link
Member

owen-m1 commented Mar 30, 2019

@sumitg-3767 Ok, then you can even remove the event listener in it's own callback rather than in onEnd. You know that sorting will stop once dragend is fired so it shouldn't make a difference.

var dragEnd = function(evt) {
	if (!dropped) {
		item.parentNode.removeChild(item); // Remove dragged item from DOM
		item.removeEventListener('dragend', dragEnd); // Remove event listener
	}
};

@smg99
Copy link

smg99 commented Apr 11, 2019

Thanks a lot @owen-m1 . Any schedule for next-version branch to be released? Would it have breaking changes?

@owen-m1
Copy link
Member

owen-m1 commented Apr 11, 2019

@sumitg-3767 No schedule as of now, but I am almost done a commit that will allow all items to animate rather than just 2, so just a few more months. There are no breaking changes.

@ghost
Copy link

ghost commented Apr 17, 2019

Correct me if I'm wrong here, but is there any need for the custom 'dragend' event listener? I was able to achieve this same functionality like this:

let droppedOutside;

el.addEventListener('drop', () => { droppedOutside = false; });

new Sortable(el, {
    onStart: () => { droppedOutside =  true; },
    onEnd: ({ item }) => {
        if (droppedOutside) {
            // Item is dropped outside
            item.parentNode.removeChild(item);
        }
    }
});

EDIT: The 'drop' event doesn't seem to fire at all when in fallback mode or when using a touch interface (even without fallback). The only way I could get this to work consistently is like this:

new Sortable(el, {
    onEnd: ({ item, originalEvent: { clientX, clientY } }) => {
        const { top, right, bottom, left } = el.getBoundingClientRect(),
            droppedOutside = (clientY < top || clientX < left || clientY > bottom || clientX > right);

        if (droppedOutside) {
            // Item is dropped outside
            item.parentNode.removeChild(item);
        }
    }
});

A constant could also be defined to grow the boundaries depending on the amount of precision desired.

@JivanRoquet
Copy link
Author

@jeffm24 at first sight, it looks like your initial example lacks providing the user with a feedback indicating that the item is going to be removed, until he actually drops it out of boundaries.

Your second example is a perfect illustration of why we would welcome the addition of such an option, allowing doing this with much less code and in a much less error-prone way.

@ghost
Copy link

ghost commented Apr 18, 2019

@JivanRoquet My point with the initial example was really only to provide an alternative to the temporary workaround implementation that was mentioned above, as it seemed to be doing extra work that was not needed. To be more safe, it wouldn't be very hard to toggle a confirmation dialogue of some sort inside of the "if (droppedOutside)" check rather than immediately removing the element, but that's kind of besides the point.

I do agree that it would be much preferable to have something built in and less error-prone than my second example, but I just wanted to share it because it's the only workaround that I've found that seemingly covers all edge cases for now.

EDIT: As for the actual proposal, maybe it would be more flexible to have an onSpill handler that would fire before/instead of onEnd when an element is dragged out of the current container. This would allow the user to decide exactly what would happen on spill, rather than having to define multiple different options related to it. Behind the scenes, the code could just check if 'onSpill' is defined in the options and, if so, perform the same getBoundingClientRect check to decide whether or not to fire it.

@owen-m1
Copy link
Member

owen-m1 commented May 24, 2019

@JivanRoquet @jeffm24 Just to let you know - you can use the Sortable.js file in the next-version branch to get this feature. It has the new OnSpill plugin included.

@owen-m1 owen-m1 moved this from To do to In next-version branch in Tasks May 24, 2019
@owen-m1
Copy link
Member

owen-m1 commented Jun 1, 2019

This feature has now been merged into the master branch, and therefore I will close this issue.

@owen-m1 owen-m1 closed this as completed Jun 1, 2019
@JivanRoquet
Copy link
Author

JivanRoquet commented Jun 4, 2019

Thanks for the work on this one

@yagnikvarma
Copy link

OnSpill plugin is not working with multi drag. Is there a way to achieve revertOnSpill with multi drag?

@owen-m1
Copy link
Member

owen-m1 commented Sep 11, 2019

@yagnikvarma Please open a separate issue for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Tasks
1.10.0-rc
Development

No branches or pull requests

4 participants