justspamjustin / bossview Goto Github PK

It would be good if _getSubViewRenderConditions would use _.result to resolve the "subViewRenderConditions" property. In the code for our BossView, we want to specify other methods in the View as the render condition methods, but can't, since you can't reference other methods in the same object when creating an object in code.

subViewRenderConditions: {
     view1: this.testCondition1,
     view2: this.testCondition2
}

The above case does NOT work, as testCondition1 and testCondition2 are methods on the currently-being-defined view. You would need to do something like this:

subViewRenderConditions: function() {
     return {
          view1: this.testCondition1,
          view2: this.testCondition2
     }
}

This WOULD work, if _getSubViewRenderConditions used _.result.

Right now it only uses this.model or this.collection when setting up the view.

This makes no sense if you have, for example, a collectionview subview that doesn't require the same collection OR more than one collectionview subview.

Is there a way to set this up currently (if so, it's non-obvious and not documented).

Right now the code hooks the subview rendering into the trigger for the parent render.

This is fine, but it means there's no way to be sure every subview has rendered before calling some code.

A simple event on the parent to say it's done would help a LOT in this situation.

Hi there! I saw your project via a tweet and had to check it out. It looks pretty sweet! I like the direction you're heading. The declarative child view events are especially awesome. Nicely done on that. :)

I did want to suggest a better version of sub view in a containing element sample, though. Marionette's Layout provides exactly this, though it may have a slightly different focus than what you're showing.

But to keep the code comparison a little more clean, I'd recommend updating that sample to look like this:

var SubView = Marionette.ItemView.extend({
  template: function() {
    return 'something';
  },
});

var TopView = Marionette.ItemView.extend({
  template: function() {
    return '<div class="some-container"></div>';
  },

  regions: {
    someContainer: '.some-container'
  },

  onRender: function() {
    this.someContainer.show(new SubView());
  }
});

It's not quite as condensed as your configuration, still, but it gives a more accurate depiction of what Marionette can do.

Right now I'm reduced to the following for dynamic updating

_.each(dynamicListOfViews, function (itemData, name) {
    this.options.subViews[name] = function () {
        var model = new MyModel(itemData);
        return new MyView({model: model});
    };
}, this);
this._initializeSubViews();
this._initializeChildViewEvents();
this._initializeSubViewEventBubbling();

This is obviously a bit rough.

A simple insertView/addSubView method could takethe view (or object for a list of views) and then do the needed setup.

Would create much cleaner code when dealing with dynamic subviews (that may not be directly from a collection for example).

Sure I could make a contrived collection and use some form of CollectionView but then I end up writing a whole lot of other boilerplate code that doesn't really solve the issue and I end up not at an ideal solution anyhow :(

Hi again!

I took a look at your code quickly and noticed that a lot of the code you have in here is managing your list of sub views. There seems to be a lot of loops and callbacks with the eachSubView method, and other bits of code that look very familiar to what I've built in to Backbone.Babysitter already.

I think you might benefit from using Backbone.BabySitter, honestly. I think it could help to reduce your maintenance costs and code by re-using the part of Marionette that already handles management of sub-views, looping through them, and manipulating them.

Really what this comes down to is saving you some code. Since Babysitter is already part of Marionette, and you've built a great extension to Marionette, it makes sense to try and take advantage of what's already there.

https://github.com/marionettejs/backbone.babysitter

Let me know if you have any questions about BabySitter, or if you don't think it would fit your needs. I think it will off-hand, but I haven't really fully understood everything you're doing, yet.

Problem :

Calling destroy on BossView object doesn't call destroy on subviews, resulting on a potential memory leak.

Subviews' "onDestroy" never get called, and in the Chrome Backbone Debugger we can see that the subviews get destroyed, in a way (?!)(but this debugger is not always accurate), but all their own children are kept alive.

Solution :

I've added destroy as the remove call

destroy: function () {
  Marionette.ItemView.prototype.destroy.apply(this, arguments);
  this._destroySubViews();
},

_destroySubViews: function () {
  _.each(this.initializedSubViews, function(subView) {
    subView.destroy();
  });
}

It does the trick for me but I don't have the time to make the unit test and a pull request. Add if you think it's relevant.

It'd be nice if I could

require("bossview")

Thanks