angular-ui / ui-sortable

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

Fixed to work with deep descendants #492

Closed yenoh2 closed 7 years ago

yenoh2 commented 7 years ago

In trying to use ui-sortable with Angular Material tabs, I found that it didn't work for two reasons. First you can't have two directives with an isolated scope on the same element. Second, ui-sortable didn't support items that aren't a direct decendant. I modified the "getElementScope" function to get the applicable scope for the draggable item, if the normal code results in null. I also wrapped the md-tabs directive in a div containing the ui-sortable directive to circumvent the two directives with isolated scopes issues.

As far as I can tell, this works. Can you see any issues with my change?

See the following example: http://codepen.io/yenoh2/pen/rWxxVJ

This seems to be related to "Using ui.sortable with angular ui.bootstrap.tabs have a weird interaction #248"

thgreasi commented 7 years ago

Great! Can you resolve the linter errors that travis-ci seems to complain about? I will try to review during the weekend.

thgreasi commented 7 years ago

I'm also thinking of removing the isolated scope thing. It should be safe, since we are not using the scope at all.

thgreasi commented 7 years ago

I just remembered that the main issue we had was canceling on transcluded directives. Maybe this is an opportunity to finally add that feature.

yenoh2 commented 7 years ago

Yeah I'll look into it tomorrow.

On Nov 28, 2016 4:21 PM, "Thodoris Greasidis" notifications@github.com wrote:

Great! Can you resolve the linter errors that travis-ci seems to complain about? I will try to review during the weekend.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/angular-ui/ui-sortable/pull/492#issuecomment-263298333, or mute the thread https://github.com/notifications/unsubscribe-auth/AWduHc6Yoy-r3NHpMLTnDWk8ARX3kWOmks5rCvFbgaJpZM4K94rr .

yenoh2 commented 7 years ago

Yeah that will be nice to be able to use with other directives.

On Nov 28, 2016 4:31 PM, "Thodoris Greasidis" notifications@github.com wrote:

I'm also thinking of removing the isolated scope thing. It should be safe, since we are not using the scope at all.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/angular-ui/ui-sortable/pull/492#issuecomment-263301303, or mute the thread https://github.com/notifications/unsubscribe-auth/AWduHSuprhyowXFF03T1vjDRKAEgLmP1ks5rCvO6gaJpZM4K94rr .

yenoh2 commented 7 years ago

What was it doing or not doing? We're you using ui-sortable directly on the transcluded directive?

On Nov 28, 2016 5:58 PM, "Thodoris Greasidis" notifications@github.com wrote:

I just remembered that the main issue we had was canceling on transcluded directives. Maybe this is an opportunity to finally add that feature.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/angular-ui/ui-sortable/pull/492#issuecomment-263327270, or mute the thread https://github.com/notifications/unsubscribe-auth/AWduHU9U4t2tjFabD2b8RxeOD18te8ibks5rCwgugaJpZM4K94rr .

thgreasi commented 7 years ago

The last two years I've tried properly implementing canceling with transcluded directives but always some extra commitments appeared and I had to stash everything. Tryied both your approach and also on the same element after removing the scope from ui-sortable. We can try to get it right this time. I was thinking about an option to specify the transclusion target that hosts the sortable items.

yenoh2 commented 7 years ago

I made the corrections for the linter errors. How do I re-check?

thgreasi commented 7 years ago

Great! I think that the tests aren't running because your branch is conflicting with the current master (I released a fix yesterday). How about rebasing to the current master (or opening a new PR, if that works better for you)?

thgreasi commented 7 years ago

Closing this in favor of #494