valor-software / ng2-dragula

Simple drag and drop with dragula
http://valor-software.com/ng2-dragula/
MIT License
1.91k stars 430 forks source link

FormArray support? #600

Closed dotjoe-zz closed 6 years ago

dotjoe-zz commented 7 years ago

If you try to use a FormArray's controls property as a [dragulaModel] the form's change detection will not work since...

https://angular.io/docs/ts/latest/api/forms/index/FormArray-class.html

To change the controls in the array, use the push, insert, or removeAt methods in FormArray itself. These methods ensure the controls are properly tracked in the form's hierarchy. Do not modify the array of AbstractControls used to instantiate the FormArray directly, as that will result in strange and unexpected behavior such as broken change detection.

I worked around this issue by using a FormArray wrapper as my [dragulaModel] that implements a splice using the FormArray's insert and removeAt methods.

Are there any plans to support this common scenario? Or, is there another way to handle this?

nayfin commented 7 years ago

Would you be interested in making a plunk of what you were able accomplish? I need to implement something similar and it would be helpful to build on what you have accomplished.

cormacrelf commented 6 years ago

No plans to support FormArrays directly. It might be useful documenting how to do it though. Please post your solution, @dotjoe.

cormacrelf commented 6 years ago

My apologies, I just read the PR. Thanks for submitting it. It can be documented.

cormacrelf commented 6 years ago

If anyone's still wanting this, I've read the PR and it goes a bit too deep, and would require way too much testing. Maybe not this release.

dotjoe-zz commented 6 years ago

I closed that PR because I wound up going much deeper on my fork. Got it to work with FormArray and also added some hooks to work with virtual scroll https://github.com/rintoj/angular2-virtual-scroll

cormacrelf commented 6 years ago

I’m guessing you made the whole array implementation swappable? That would be nicer than trying to guess what people are supplying. If it is, I could integrate that into v2.

How did you tackle virtual scroll? I don’t immediately see why it should require a fork. Especially with v2.0, which doesn’t do any internal mutation anyway, just shallow clones then sends you the splice results.

I would like to keep complexity on this repo to a minimum, so if your diff is +2k LOC it isn’t worth the maintenance for me. OTOH, I don’t use the library any more, and I don’t really care, so you could maintain it if you like.

————

A final point —

This library has a lot of silently happy users, almost none of whom are doing anything remotely complicated like you are. The basics are pretty fragile still, even with tests, and generally it’s not a great platform for advanced features. Nobody knows why it works with change detection, or when it doesn’t. The docs are involved enough as it is, and users are easily confused (see the classic blunders and half the issues.)

I built angular-skyhook to satisfy almost every advanced use case.

My point is, it’s would be more worthwhile building advanced features over there, and it wouldn’t make this library more difficult to learn or use.

dotjoe-zz commented 6 years ago

Virtual scrolling isn’t supported out of the box because it looks for the drop index by checking the position of the dom element. I put in a hack to see if the model can translate the drop index since it will know the current offset. Similarily, for formsarray I just test for the known methods (insert, removeAt, etc) and used them to perform the splicing, no actual forms dependency.

I will have to check out skyhook, looks like what I need. I never really had any plans to try and merge my ng2-dragula fork back in.

One other thing I had to work around was handling a multi-item drag and drop. It’s really just a normal single item drag except I modify the drag preview with a count of the selected items and then handle the drop event.

dotjoe-zz commented 6 years ago

And my fork is based on v1.3.1, didn’t realize v2 was out

cormacrelf commented 6 years ago

That's ok. I don't think merging is a great idea. Edit: your fork is definitely public.

I'm eventually going to build some example code for multi-item in angular-skyhook-card-list. It'd be essentially the same, just render something different in your <skyhook-preview>, and write your own SortableSpec. You could even render an ngFor of all the items you decided to take with you.

I'm going to close this now, but it's still got the "future reference" tag which the issue template tells people to look at.