gmac / backbone.epoxy

Declarative data binding and computed models for Backbone
http://epoxyjs.org
MIT License
614 stars 89 forks source link

Pass ItemView instead of attaching a view to a collection #45

Closed sdemjanenko closed 10 years ago

sdemjanenko commented 11 years ago

Presently one is limited to one view per collection. This change lets users render the same collection into multiple views.

It supports the old API of collection.view. If you would like to use a collection in multiple views, you can now set the ItemView property in each CollectionView (don't set a view on the collection).

jhohlfeld commented 11 years ago

Indeed a valuable feature. Alternatively it could be achieved by just setting the collection instance's view member at run-time. Only thing to care about: you have to do it from within the constructor.

For instance:

var ListView = Backbone.Epoxy.View.extend({
    initialize: function() {
        this.collection = new ListCollection({renderView:ListItemView});
    }
});

will give a collection the ability to render just fine. For example:

var ListCollection = Backbone.Collection.extend({
    model: ListItemModel,
    view: null,

    constructor: function(options) {
        this.view = options.renderView;
        Backbone.Collection.apply(this, arguments);
    },
});
sdemjanenko commented 11 years ago

I feel like the itemView option would be cleaner. It has always seemed odd to me that a collection has a view Constructor attached to it. While I don't want to break existing functionality that uses it, I feel that having an ItemView option would help me write cleaner & more understandable code. I would stop using collection.view entirely in favor of ItemView.

gmac commented 10 years ago

Hmm... I see what you're saying. In general, I don't like the existing design pattern where a Collection (model structure) defines a view reference. I'll review your changes...

abrindam commented 10 years ago

+1 for this. Just started using the framework and loved it, but this jumped out at me almost immediately. At least for me, my Models generally know nothing about the Views that render them.

@sdemjanenko Could you maybe post a snippet giving an example of how your fix would be used?

sdemjanenko commented 10 years ago

Sure @abrindam. Here is an example

var BookDetails = Backbone.Epoxy.View.extend({
  tagName: "li",
  ...
});

var BookList = Backbone.Epoxy.View.extend({
  tagName: "ul",
  ItemView: BookDetails,
  bindings: {
    ":el": "collection:$collection"
  },
  ...
});

var books = .... // some collection
var loadedBookList = new BookList({collection: books});

$("#bookList").append(loadedBookList.$el);

There is no need to bind a view to the books collection. One simply registers the BookDetails view as the ItemView.

Prinzhorn commented 10 years ago

Is there a particular reason you upper-cased ItemView? My OCD can't handle it.

sdemjanenko commented 10 years ago

I upper-cased it since you pass the view factory as the value for that key (not a view instance).

Prinzhorn commented 10 years ago

I upper-cased it since you pass the view factory as the value for that key (not a view instance).

Same for model on Backbone.Collection, which is lower case.

gmac commented 10 years ago

Okay, sorry for the REALLY long delay in reviewing this... I've been very busy. I'm all for this change in pattern; the View-on-a-Collection thing has always been a terrible relationship that was defined haphazardly early on and then never changed. Pull and reviewing now.

gmac commented 10 years ago

You got it: see 6ba8cc1. Thanks for pressing this issue until it was resolved! Oh, please note though – I do agree with @Prinzhorn on the item view reference name: it's been implemented as a view property called itemView.

sdemjanenko commented 10 years ago

Cool. Im glad this functionality finally got added! Thanks for all the hard work @gmac.

Prinzhorn commented 10 years ago

Yeah, thanks!

Prinzhorn commented 10 years ago

I think just found a case which only works with the "legacy" way: you can use multiple data sources on the same view. E.g. a collection and a otherCollection which render to different parts of the same view. I don't use this myself (since you should just use two views then), but it's certainly something that you should have in mind before dropping the old view property in a future version.

See http://epoxyjs.org/tutorials.html#binding-sources for reference

Edit: Also I just found another thing in our codebase while migrating to the new epoxy version: I have an abstract DetailsView which can be used for two different types of models, because their details view is similar. In this case it feels wrong to add the itemView property, because it's only needed in one of the cases, where the model has an embedded collection inside.

adamyonk commented 10 years ago

I keep getting an error that says Binding "collection" requires a Collection with a "view" constructor.. Is this correct? After this merge, I thought you could set an itemView on the same View that holds the collection and it would use that?

Prinzhorn commented 10 years ago

@adamyonk Are you sure you're using the right version? The error message you mentioned doesn't exist in the source anymore (first lines here https://github.com/gmac/backbone.epoxy/commit/e1ddcc1a28a1e26c3ede22f206e68ca925b772e3)

adamyonk commented 10 years ago

@Prinzhorn Ah, I had bower set to pull down v1.2, but there was a mixup somewhere. I have the latest now and it works great.