feat(sortable): allow extra element before/after ng-repeat

[tomyam1]

This fix allows to have extra elements before/after ng-repeat (only if they are not ng-repeat themselves).

See demonstration at: http://codepen.io/anon/pen/VYgOLv

It passes all the original tests. To test that it works with added items, I duplicated sortable.e2e.spec.js into sortable.e2e.extraElements.spec.js and added a dummy div before and after the ng-repeat in every test.

Fixes #41, Fixed #177, Fixed #98 and Fixes #207

[coveralls]

Coverage remained the same at 100.0% when pulling 8ec7b03 on tepez:extra_elems into 704cc55 on angular-ui:v0.14.x-dev.

[thgreasi]

@tepez ty for your effort 👍
Will review and merge asap!

[tomyam1]

my pleasure @thgreasi

[thgreasi]

How about changing the "dummy" elements to be li's?

[tomyam1]

Sure @thgreasi . Maybe we should wrap all the specs in sortable.e2e.spec.js in a function, e.g. function runTests(beforeElement, afterElement) that will run all the specs, with beforeElement added before the ul element and afterElement after it.
We could than avoid the current duplication of code by doing

runTests(); // run the tests with no added elements
runTests('<li></li>', '<li></li>'); // run the tests with added elements

[thgreasi]

👍 but I also like the fact that the extra element tests is currently in a separate file.
Feel free to proceed as you wish.

[thgreasi]

Sorry this takes so long. I will try to review&merge during this weekend.

[coveralls]

Coverage remained the same at 100.0% when pulling 58c6c7b on tepez:extra_elems into 704cc55 on angular-ui:v0.14.x-dev.

[tomyam1]

No problem @thgreasi. I'm not in a hurry for this.
You where right to ask to use li instead of div for the extra elements.
It seems that using li breaks 4 tests. I could not determine why. Can you give it a look?

[thgreasi]

It looks like that drag&revert breaks the angular comments order, in the case that:

• helper or placeholder are used
• an extra element exists at the end

[thgreasi]

I created a new branch with a commit that fixes your test cases.

BUT, this is still not ready to go since it fails when I add an extra test-case with conneted sortables
In sort:

• two connected sortables, having extra ng-repeat siblings
• using clone and palceholder options
• doing a drag and revert

Please merge this back in your branch so we can continue collaborate on this.

[thgreasi]

@tepez I think that this implementation will break some use cases, but I need your thoughts on this as well.
In some questions/issues in this repo, people were using the items option in order to disable the dragging of some elements of the repeater, for example the odd ones. Unfortunately this use case will break and I'm sorry that an e2e test for this case is missing.

I think that we should introduce a ui-items option to hold a selector, that should match the ng-repeat elements.

[thgreasi]

Moreover, we should also find a good way to document the facts that:

• only the ng-repeat elements should be allowed to be sortable
• any extra element will have to be locked in place, since it is not part of the ng-model.

All the above + the fact that in most use cases I have seen do far (including my first attempt using this repo), the extra elements will semantically look odd. That made me mark the related issues an a wontfix. (What does a button want next to my shopping list items.)

[tomyam1]

@thgreasi I will try to go over it this weekend.

[thgreasi]

Cool @tepez .
It's Easter here in Greece, so I guess I will have some time this weekend as well.
If you could merge my PR, I could try to fix the connected sortables issue.

Any feedback on the ui-items option is welcome. I guess this should have the same initial value as items ([ng-repeat]...)`.

[tomyam1]

@thgreasi I actually think the current implementation handles custom items. This spec shows it, no?

The reason I think it works is we always use ng-repeat to generate the items from the model. Even if some of the items are locked, e.g. are not matched by the items option, we still consider them when we determine the index of an item.

Does this makes sense?

[thgreasi]

@tepez Yea, I'm really sorry about that....
I realized that you already had that test-case (and how the trick is done) this afternoon.

I'm really sorry for the inconvenience. I think that this is in good shape to be merged as soon as the tests pass.

I have made some commits that fixes the failing tests cases (wanted to keep track of all this in one PR).
It also includes some refactoring so that we can at some point get rid of sortable.e2e.extraElements.spec.js and run all the spec files both for the simple and the case that extra elements also exist.

Lastly, an option like ui-items (a selector that would match DOM elements with ng-model items) would allow for greater control and possibly more than one ng-repeats inside the ui-sortable directive, but I guess we should open a new issue for that.

[thgreasi]

Merged with 36badd1.
@tepez Thanks for your effort on this great commit 👍