Need extra reviews to push angular1.2 as master

[thgreasi]

We need some extra collaborators (cough ex. @ProLoser) to review angular1.2 branch.
If it seems solid we could push it as master since the current master does not work with AngularJS 1.2.x.

There already exists an issue #48 that got bloated by demand. PR #56 was then open to ease the merge and was later merged.

Of course, it would be wise to keep a seperate 1.0.x branch as well.

[ProLoser]

Holy jeezus that is a long Pull Request discussion.

I didn't read the whole thing, but at the point you were reaching (drag+drop then reverting and having angular reflect changes) have you guys considered just rolling this natively?

Primarily, I would think the focus is:

1. Clone DOM element on drag-start and make it position: fixed
2. Move cloned DOM element around as mouse moves
3. Calculate final location on drag-end and new item order
4. Destroy clone and update models

I'm not saying this is necessarily easy, I'm just wondering if we're going through so much effort, is it necessarily harder than what we're already doing?

[ProLoser]

@thgreasi I'm going to have to trust you guys. If it works it's your judgement on pushing this into master.

One note: https://github.com/angular-ui/ui-sortable/blob/angular1.2/src/sortable.js#L42

I'm not sure 100% I'd trust this solution. If you remove AND add in the same $scope.$apply(), my guess is that the order won't be updated properly since the length doesn't change. Let's also hope this doesn't throw errors if the value is a non-array.

You should probably also slap a huge warning on the readme that this only works with arrays (because people are stupid).

[CyborgMaster]

So is this enough to get his merged into master? Or should we get a few more reviewers?

[bcronje]

@CyborgMaster @thgreasi There's still a bug in the latest version. If you drag an item over a new slot and then move back to it's original position, effectively cancelling the operation, sortable still messes with the ng-repeat comments. So if you then do the same thing but actually drop it you end up with a correct model but incorrect rendered view. A quick fix for this is to re-attach the savedNodes inside the stop event as well if no sorting happened, e.g.

callbacks.stop = function (e, ui) {
              // If the received flag hasn't be set on the item, this is a
              // normal sort, if dropindex is set, the item was moved, so move
              // the items in the list.
              if (!ui.item.sortable.received && ('dropindex' in ui.item.sortable) && !ui.item.sortable.isCanceled()) {
                  scope.$apply(function() {
                      ngModel.$modelValue.splice(
                          ui.item.sortable.dropindex, 0,
                          ngModel.$modelValue.splice(ui.item.sortable.index, 1)[0]);
                  });
              } else {
                  savedNodes.detach().appendTo(element);
              }
            };

[thgreasi]

@bcronje can you provide a fiddle/pen with that case?
I created some tests, but it seems I can't find the case you describe.
Here is a pen with the latest version.

[bcronje]

@thgreasi I just tested again with latest version of angular1.2 branch and the issue mentioned previously seems to have been fixed. I was unable to reproduce it in my own code not using your pen, so all is good.

PS Thank you for providing the pen. Very nice to have a quick starting point.
PPS Your pen actually pointed to the master branch and also Angular 1.0.7, but I updated a fork of that pen to the angular1.2 branch and latest AngularJs 1.2.5 version and still couldn't reproduce it.

[pennersr]

Has this been tested with the new ng-repeat="item in items track by item.id" functionality offered by ng-repeat ?
When using connected lists, and dragging an item from list one to two, and then back again, things will be messed up. Specifically, the item being dragged is somehow duplicated. Without "track by" this works fine, so I suspect that the savedNodes workaround is not "track by" proof. I'll see if I can come up with a fiddle demonstrating the issue...

[thgreasi]

@pennersr Here is a pen using track by app.id, but couldn't reproduce the behavior you saw.

PS: please use the demo pens found in READEME.MD. It only took me 5 mins to create this forked pen.

[pennersr]

I tried to recreate this in a pen but the problem did not show, which finally led to a bug in my own code. My apologies! All is working fine now ...

[julien-gm]

Hello,

thanks for the great job. However, I have 2 issues to report for the angular 1.2 branch :
1- If the helper is set to "clone", it's not removed after the update.
2- If there are "static" items in the list, the item index is not good

You can check this out here: http://codepen.io/anon/pen/AKnaf

Thanks again

[thgreasi]

@julien-gm nice to have your feedback.
Please also open a new issue to track the progress. Also reference this issue as well.
I will possibly start working on it tonight.

PS to all in this thread:
In my opinion angular1.2 branch has progressed/matured enough. I might also say that it's state is better than the master branch, since there are some tests that pass only on the angular1.2 branch. Moreover, it's time to focus on the current release of the framework.

[thgreasi]

@julien-gm I just opened a new issue focusing to the helper: "clone" part of your example.
Thanks for pointing out.

The reason I had to split your example, is that you are using extra elements inside the directive (aside from the ng-repeat).
As noted in the README.md (in the developer notes section) and in tickets #41 and #98:

ui-sortable element should only contain one ng-repeat and not any other elements

[thgreasi]

Since angular1.2 branch supports more jquery-ui features than master (and is better tested), I opened PR #116 to merge it.
I think we waited enough and not supporting angularjs v1.2 within the master branch is causing confusion (as can be seen by the quantity of closed question-issues).

[thgreasi]

Closed by #116
Many thanks to @CyborgMaster and all issue reporters.

[CyborgMaster]

Thanks @thgreasi! Sorry I haven't been around as much lately to help with continued development. Now that it's working for our project, I haven't had any more time to dedicate. Thank for all your continued hard work.

[bcronje]

@thgreasi thank you for pushing progress here. Much kudos to you!