marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.07k stars 1.27k forks source link

CollectionView / CompositeView should have an optional filter method #1596

Closed samccone closed 9 years ago

samccone commented 10 years ago

Collections have a comparator, and we now respect that in the view layer. However the next most common tasks for lists is to filter / reduce the set of results shown to the user.

There are currently several strategies, such as resetting with a new collection, using a third party plugin like backbone filtered collection, or to hack into addChildView.

We should provide a formal way for people to do this to remove the burden from the developer.

Marionette.CollectionView.extend({
  filter/omit/where: function(model, ChildView, index) {
    return true / false
  }
})
jamiebuilds commented 10 years ago

Shouldn't this be done on a collection-level?

samccone commented 10 years ago

the filtering is for presentation only so thus a view concern imho

cmaher commented 10 years ago

I'm personally a fan of wrapping collections. It gives you the full benefit of working with collections and it keeps concerns nice and separate. The backgrid project put some sorting behavior into the views and made it very difficult to hook into in a consistent fashion (otherwise, it's a great project). I can see this working out very nicely for simple use cases, but I don't think the view is a good place to do heavy filtering.

disclaimer: I'm working on open-sourcing some work my company has done on collection filtering.

jamiebuilds commented 10 years ago

@samccone From what I've seen, most people do it on a collection level by creating a duplicate collection that is filtered

samccone commented 10 years ago

yep that is how i do it also @thejameskyle but we can help people and remove this step by putting the responsibility of the collection view filters on the view and not on the collection.

Just my thoughts tho

ahumphreys87 commented 10 years ago

I think I'd also prefer something collection level to keep it consistant with the comparator property, but backbone doesn't provide a filter property :( so I think having something view side will make it more obvious to people how to do this.

The downside of providing a restricted collection is that the full collection will probably need re-rendered each time something changes. If we implement view side we can just add/remove the specific models that we need to

On Wednesday, 9 July 2014, Sam Saccone notifications@github.com wrote:

yep that is how i do it also @thejameskyle https://github.com/thejameskyle but we can help people and remove this step by putting the responsibility of the collection view filters on the view and not on the collection.

Just my thoughts tho

— Reply to this email directly or view it on GitHub https://github.com/marionettejs/backbone.marionette/issues/1596#issuecomment-48534122 .

jfairbank commented 10 years ago

I'm mixed on this. I've done this a lot on the collection-level as well, but I really do like the idea of filtering at the view level with a filter-type method. It explicitly expresses what exactly should be displayed for the view. I think an implementation that handles rendering similar to @ahumphreys87's suggestion would be definitely worth a look.

cmaher commented 10 years ago

I can see this being useful for common display cases, but separating it from the collection removes a ton of functionality. What if someone then wants to act on the filtered subset? Handling this information on a collection lets you use all the standard backbone tools. Handling it on the view only gives you display functionality.

samccone commented 10 years ago

@cmaher this is the use case

a collection of people... filtered by hair color

but i want to push new people to a single collection and have my view update accordingly without having to reset my collection on the view.

View layer filtering is a HUGE win for me I think.

jamiebuilds commented 9 years ago

What if someone then wants to act on the filtered subset?

:+1:

There's nothing to gain from putting it on the view layer, but a lot from the collection layer

ahumphreys87 commented 9 years ago

I think my preference would be to have something on the collection as well - consistent with comparator. Off the top of my head we have a few options:

  1. We can filter based on a custom filter option on the collection - document that users would need to add this, this could then form the basis of the logic in addChildView.
  2. Create a Marionette.FilteredCollection class.
  3. Provide a mixin to extend collection.

I agree with @samccone that we should be able to add to the original collection and have the view decide whether to show the new child.

Option 1 would give us that but it doesn't really meet the case of being able to act upon the original collection, so I think we would need to combine option 1 with either 2 or 3 which could have helper functions to act upon the filtered set. This would take Marionette into unchartered territory - we would be expanding to extend other backbone elements, how does @marionettejs/marionette-core feel about that?

jamiebuilds commented 9 years ago

MARIONETTE.COLLECTION & MARIONETTE.MODEL FTW

cmaher commented 9 years ago

No real ideas or reasoning, but if Marionette.Collection and Marionette.Model do become a thing, it might be a good idea to make them mixins to the standard Backbone classes... maybe something that adds something like Behaviors?

jamesplease commented 9 years ago

Our view and architectural pieces already need a lot of work. I'd rather wait until at least v5 before adding data features to Marionette, rather than stretch our resources even thinner.

jamiebuilds commented 9 years ago

:point_up: :anguished: ... :pensive:

samccone commented 9 years ago

yeah... I still think having a filter optional method in marionette would bee SOOOO nice

jamesplease commented 9 years ago

Yup, agreed. I also agree that it should happen in the view layer.

cmaher commented 9 years ago

https://github.com/andreypopp/backbone.projections and https://github.com/jmorrell/backbone.obscura both offer rich filtering abilities. I think any filtering in the view will be too limited and difficult to grow without negatively impacting the api.

samccone commented 9 years ago

I have had tons of perf problems with both of these @cmaher

cmaher commented 9 years ago

Same (at least with projections), I was more offering them as examples of what can be done at the collection level. The main problem I've seen is that they don't pass along event options and therefore don't allow you to optimize for performance when necessary.

rhubarbselleven commented 9 years ago
  1. We can filter based on a custom filter option on the collection - document that users would need to add this, this could then form the basis of the logic in addChildView.
  2. Create a Marionette.FilteredCollection class.
  3. Provide a mixin to extend collection.

As these style of things are usually a function with unknown logic and would also need to listen to state changes in the originating collection (@samccone died his hair purple), 2. Seems the most viable to me.

JProgrammer commented 9 years ago

This functionality would be greatly appreciated, most apps I build need something like this.

I currently do this by overriding addChild and getChildView it would be a lot easier if Marionette provided it nativly.

samccone commented 9 years ago

most apps I build need something like this.

I agree, this is why I think the feature is not really bloat since we would be formalizing a pattern that people are already doing in their apps. (Like behaviors for mixins)

RohovDmytro commented 9 years ago

most apps I build need something like this.

+1

sdnetwork commented 9 years ago

I think in some cases this functionnality is essential, for example if your model is shared on multiple view and in one (a grid for example) the user want apply a filter, the others view don't have to be impacted by this filter.

stephanebachelier commented 9 years ago

I agree with @sdnetwork use case. It's really common.

samccone commented 9 years ago

thoughts @marionettejs/marionette-core now that we have heard from more of the community?

jamesplease commented 9 years ago

Let's prototype some things. I think I can do it really nicely from the View, so I'm going to look into that. Someone else should make up an example of it from the data classes.

p.s. it might be a bit before I work on the example. Lots of other things in the works!

cmaher commented 9 years ago

@jmeas I can work on a prototype of filtering from the data. (it might be a bit of time for me too)

cmaher commented 9 years ago

I have a sample project showing some very simple filtering happening at the collection level: https://github.com/cmaher/marionette-filtered-collection-view, while tying that collection-level filtering to the view's control.

jackocnr commented 9 years ago

I've struggled with this in both of the Marionette apps I've built so far. One example is when you're displaying a list of soccer games grouped by time (this week, next week and upcoming), and the user can delete any item, and can add new ones.

In the first implementation, we filtered the collection in the controller into 3 sub-collections, and passed each of those to it's own CollectionView, which breaks the auto-update of the CollectionView on add/remove. In the second attempt, we just used a single CollectionView and did the filtering in there by overriding appendHtml to append each item into the right group in the markup, but this seemed hacky and had it's own problems (handling empty states etc).

Goals as I see them: auto-update magic on add/remove (avoid just re-rendering the entire list). emptyView works as expected. If possible: ability to display a count at the top of each group.

cmaher commented 9 years ago

This seems to be something a lot of people want, so I will withdraw my opposition to this feature. If @marionettejs/marionette-core is :+1: to this, then I am fine with it.

coderek commented 9 years ago

This is definitely an useful feature as it allows collection states to be consistent.

Here is my temporary solutions for anyone interested. It extends CollectionView and Model.

https://gist.github.com/coderek/93e74dd5183814a76fa2

cmaher commented 9 years ago

Resolved by #2261

betovelandia commented 9 years ago

Just saved my day with this, thanks @cmaher

samccone commented 9 years ago

its a dream

jackocnr commented 9 years ago

Loving this feature guys, thanks! Some feedback for my particular use case:

1) I feel like I need some kind of re-filter option for when I change a model in such a way that would affect the filtering. So currently we automatically update the collectionView for adding and removing models from the collection, but if you change an attribute on a model that would mean that it would get filtered differently, nothing happens. I think it's right that it doesn't auto re-filter on every model change, but what about if you could do something like collectionView.refilter() or collection.trigger("refilter") or whatever? The workaround I'm currently using is to remove the model from the collection and then re-add it.

2) I'd like to be able to display a count at the top of the collectionView. One flexible way to solve this would be to make the filtered models available to the view instance e.g. onRender: function() { this.filteredModels.length... }

UPDATE: Oh I see I can use this.children.length - that'll work!

UPDATE: Hmm this.children.length does not work in the following 2 situations: 1) In serializeData - I guess because this is done before processing the child views. Would be solved by pre-filtering the collection and storing a reference to those filteredModels. Workaround: use onRender instead. 2) In my updateCount callback for my collectionEvents: {"add remove": "updateCount"} - I guess because we haven't yet re-filtered. Would it make sense to auto-refilter on these add/remove collection events (and again store a reference to those filteredModels) before triggering any add/remove collectionEvents callbacks? Workaround: use onAddChild and onRemoveChild instead.

UPDATE: also found a bug with emptyView and filter: https://github.com/marionettejs/backbone.marionette/issues/2437

MaffooBristol commented 9 years ago

This is cool, but why's this feature not also implemented in CompositeViews?

paulfalgout commented 9 years ago

@MaffooBristol I would assume this works for CompositeView as a CompositeView just extends the CollectionView

samccone commented 9 years ago

yep it works just fine in CompositeViews

https://github.com/tastejs/todomvc/blob/master/examples/backbone_marionette/js/TodoMVC.TodoList.Views.js#L81-L106

MaffooBristol commented 9 years ago

Oh god I needed to upgrade my version of Marionette. I'm a :lemon:, sorry

Edit: Though I didn't actually know that composite views directly extended collection views so that's a handy thing to know for future

samccone commented 9 years ago

hey no problem :fish: