angular-ui / ui-sortable

jQuery UI Sortable for AngularJS
http://angular-ui.github.io/ui-sortable/
MIT License
1.26k stars 443 forks source link

[Proposal] Allow Cancel Later Than First Update() #254

Open jchamberlain opened 10 years ago

jchamberlain commented 10 years ago

When using ui-sortable with connected lists, a drag can only be cancelled in the first call to update(). However, the first time update() is called one often does not have enough information to make a decision on whether to cancel. Let me give an example.

Say I have two connected lists, where some entries may be in both lists, but where neither list can contain the same entry twice:

List 1 List 2
Chicago Los Angeles
Seattle Dallas
Seattle
Miami

If I drag "Seattle" from one list to the other, I want ui-sortable to cancel in order to prevent a duplicate. But in the first call to update(), ui.item.sortable.moved is not yet set (remove() hasn't run), so I don't know that I'm dragging Seattle. I see three possible solutions:

  1. Let the developer figure out which model is being moved and what model it's being moved into based on the DOM elements/index. This is less than ideal, in that I don't like to have jQuery in my controllers. OR...
  2. Change stop() in ui-sortable so that it detects a cancelled move and undoes the splice that remove() did, i.e., restore the models to their original condition. This would allow a cancel in the second update() or even in receive(). OR...
  3. Set additional properties on ui.item.sortable during start(), so that the developer has more information in the first run of update(), without having to query the DOM. Additional info could include the model of the sending list and model of the moved item. In the first internal update() callback, the model of the target list could also be set, if applicable. That would provide everything needed to detect a potential duplicate in the target list.

Obviously I would favor 2 or 3, or maybe even 2 and 3 both.

ORIGINAL TITLE: [Proposal] Allow Cancel Later Than First Update() OR Provide More Options

thgreasi commented 10 years ago

Have seen the connected lists canceling example in README? It uses some of the extra properties that UI-Sortable attaches to ui.item.sortable to prevent the second list to have more than 10 elements.

Obviously the use case you are describing is also feasible. To retrieve the drop target's model you can use ui.item.sortable.droptarget.scope().

thgreasi commented 10 years ago

I just added Connected Lists Without Duplicates example to README and I think it is very close to your use case.

I would like to add an extra property ot ui.item.sortable though, to hold the originating sortable list elemen,t but I can find a good name...

Names I have been thinking of:

Any suggestions?

jchamberlain commented 10 years ago

I have seen it. Thanks. That example requires using DOM attributes + jQuery in my controller to find the target model (ui.item.sortable.droptarget.attr('id') === 'screen-1'). That's what I'm doing as well, but it's the type of thing I'd rather put into a directive instead of the controller. Since uiSortable is a directive, it makes sense to let it do that work. :)

As for ui.item.sortable.droptarget.scope(), it returns the nearest scope containing the ngRepeat, which is generally not enough information to actually get the model. In the connected lists example you provide it works because you have nested ngRepeats, effectively providing an isolated scope around each call to uiSortable. When you have two different models on the same scope, the only way to get the right one is to set an id or some other attribute on the HTML node, which is what you did for the top-level ngRepeat (id="screen-{{$index}}"), or to directly parse the ngModel attribute. Since uiSortable already has the model, it seems simplest and most straightforward for it to make that model available, instead of requiring the developer to hack it.

(Edit: didn't see your post, @thgreasi. Let me look at that really quick.)

jchamberlain commented 10 years ago

@thgreasi, thanks for posting that. That is very close to what I'd like, the only difference being that it still has to know in advance/hard code in which model is being used for the ngRepeat: var originNgModel = ui.item.scope().cities;.

Now, in your example I think that's fine because--as in the other example--you have nested ngRepeats, allowing you to always have a cities model on scope. update() can always look for cities, even on different lists. But if you're not using isolated scopes for each list, you can't do that.

thgreasi commented 10 years ago

That example requires using DOM attributes + jQuery in my controller

That was a bit hacky back then. I think the new example I composed does not require DOM access. What's your opinion?

I'd rather put into a directive instead of the controller

Obviously you can wrap ui-sortable within an other directive to contain all the logic currently found in your controller. Directly parsing ngModel isn't that bad at all. It happens all the time in directives that use ng-model (and other attributes) either with scope.$eval or with (the less obvious) scope.$watch. After all, ng-model is there so that you can declaratively set your model. It's obvious that it is meant to be parsed by directives.

jchamberlain commented 10 years ago

You're right about being able to wrap it in a directive of my own, I was just hoping to not actually have to, since uiSortable can already easily get the info I need.

However, the more I think through your example, the more I like it. Unless I'm mistaken, it's still entirely dependent on nested ngRepeats, but for what I'm writing right now I think that will be fine.

As for the extra property you're wanting to add, I think sender is a good name. And if you want to add senderModel and droptargetModel while you're at it I won't complain. :)

thgreasi commented 10 years ago

I'm going to rename this issue to target adding extra properties to ui.item.sortable object. I will probably add some extra methods to retrieve the models associated with the sortings.

Please feel free to full a new separate issue about bullet 2 (aka late cancel).

thgreasi commented 10 years ago

Hmm, that doesn't feel right. I will open a new issue for the properties and rename this one to target late cancel.