CompositeView's HTML state is lost when sorting collection.

[MeoMix]

I have a CompositeView with the following logic:

onShow: function () {
    //  TODO: Marionette 2.0 broke this UI when calling .sort() -- need to fix!
    this.ui.panel.transition({
        x: this.ui.panel.width()
    }, 300, 'snap');
}

This logic provides the CompositeView with a nice slide-in effect when it is being shown.

The CompositeView's ChildViewContainer is able to be organized using jQuery UI's sortable functionality:

this.ui.childContainer.sortable({
    update: function (event, ui) {
        var playlistId = ui.item.data('id');

        var playlist = this.collection.get(playlistId);
        playlist.set({
            sequence: ui.item.index() 
        });

        //  Collections with a comparator will not automatically re-sort if you later change model attributes
        this.collection.sort();
    }.bind(this)
});

When sort is called -- the CompositeView is re-rendered which causes the transform on my panel to be lost. This is bad and unexpected! How should I fix?

Here's a video: http://screencast.com/t/hcTSyp6rGhJk

[MeoMix]

from jmeas on Gitter:

i think we need to overwrite the sortViews callback for CompositeView https://github.com/marionettejs/backbone.marionette/blob/master/src/marionette.compositeview.js#L3
to only render the children.

do this https://github.com/marionettejs/backbone.marionette/blob/master/src/marionette.compositeview.js#L79 instead of this.render();

So, I'm assuming that means:

// Internal method. This checks for any changes in the order of the collection.
// If the index of any view doesn't match, it will render.
_sortViews: function(){
    // check for any changes in sort order of views
    var orderChanged = this.collection.find(function(item, index){
      var view = this.children.findByModel(item);
      return view && view._index !== index;
    }, this);

    if (orderChanged) {
      this.render();
    }
},

This method should be overridden to call this.renderChildren() when being accessed from a CompositeView.

Making the orderChanged code not DRY seems bad. Should that logic be extracted out into a method? Or maybe an orderChanged event triggers and each type of view responds to it in a different way...? Not sure how Marionette does their inheritance elegantly.

[ahumphreys87]

👍 to this change

[ahumphreys87]

Haven't quite thought out the implementation yet though but I agree that compositeView shouldn't trash the surrounding template

[samccone]

CompositeViews are tricky tho..... since they render into the root element by default... So this change makes sense only if you are rendering into a custom container that we can clear out.

So I would say the "fix" would be to expose

if (orderChanged) {
  this.render();
}

as a named concept and method that a user can override

[MeoMix]

samccone is right... although I've never used that default functionality. :)

[MeoMix]

// Internal method. This checks for any changes in the order of the collection.
// If the index of any view doesn't match, it will render.
_sortViews: function () {
    // check for any changes in sort order of views
    var orderChanged = this.collection.find(function (item, index) {
        var view = this.children.findByModel(item);
        return view && view._index !== index;
    }, this);

    if (orderChanged) {
        var childViewContainer = Marionette.getOption(this, 'childViewContainer');

        //  If a childViewContainer is defined then do not re-render the HTML surrounding the container.
        if (_.isUndefined(childViewContainer)) {
            this.render();
        } else {
            this._renderChildren();
        }
    }
},

Would this be acceptable?

[ahumphreys87]

@MeoMix problem we have is that this could be breaking now - we can't really safely assume what people would want here. My proposal would be to provide a method that one can override so that we can maintain backwards compatibility - so like @samccone said something like:

if (orderChanged) {
  this.resortView(); // might need a better name
}

by default resortView would simply call render, however this would give you the option to override and only render children if thats what you desire.

[samccone]

@MeoMix the first model in the collection is going to be used to render the root.. so once the collection changes sort and the first model has shifted we need to rerender the entire thing and not just the first item...

and now we need to smartly render the children... if someone has a custom child container...

on top of the logic you pasted above

basically this is a nightmare, and I think you would be best served by an itemView and collectionView combo :

[samccone]

So the solution for now @MeoMix is going to be as @ahumphreys87 said and to provide you with a a method that can be easily overridden to change the default safe render action.

[MeoMix]

Would my solution be acceptable for a 3.0 breaking release? I don't understand why anyone would want their outer HTML trashed for a CompositeView when providing a ChildViewContainer.

I can appreciate the logic behind it being a breaking change... but really I see it the other way around. Currently what I'm working with in 2.0 is an undocumented, breaking change to my code and I'm trying to get it back to expected functionality. To say that modifying the code to bring it back to expected functionality is a breaking change... is a bit... odd.. but I do see where you guys are coming from.

[jamesplease]

👍 to move to 2.2

[samccone]

fixed by #1645