theironcook / Backbone.ModelBinder

Simple, flexible and powerful Model-View binding for Backbone.
1.42k stars 159 forks source link

CollectionBinder: don't sort elements on 'add' event #203

Closed amakhrov closed 9 years ago

amakhrov commented 9 years ago

Backbone triggers 'sort' each time an model was added to the collection if it changed the sort order. So there is no point in sorting dom elements on 'add' event - doing that on 'sort' would be enough.

What's more, it leads to broken collection rendering on collection.set, as sortRootEls is called after each individual add event is triggered - at this point the collection has the full set of models, but only some of them have been rendered. One of the added unit tests fails with the original code due to that.

platinumazure commented 9 years ago

What about collection.add(model, { at: 0 })? Wouldn't that possibly result in an inconsistent ordering between the collection and the DOM?

amakhrov commented 9 years ago

Good point! However, I believe for this case createEl method should be implemented in a way that it inserts the newly added element straight into the right place in DOM instead of appending and then re-sorting everything.

platinumazure commented 9 years ago

@amakhrov Good call, I agree. No reason options.at couldn't be made available there. I look forward to the tweaks :-)

EDIT: On second thought, that's really a separate issue. I'll go start that one.

amakhrov commented 9 years ago

@platinumazure all the feedback is incorporated:

platinumazure commented 9 years ago

@theironcook This should be safe to merge.

theironcook commented 9 years ago

Merged. I hope it works :)