Enforce model level filtering? #372

Closed
sindresorhus opened this Issue Jan 4, 2013 · 18 comments

Comments

Projects
None yet
6 participants
@sindresorhus
Member

sindresorhus commented Jan 4, 2013

I was made aware in addyosmani#352 (comment) that the Backbone app currently doesn't filter on a model level per app spec:

When the route changes the todo list should be filtered on a model level

That rule was introduced to show off how make use of the models. You can see in the Spine app how this is intended to work

I have a feeling there are more apps that don't follow this, seeing as we introduced that rule at a later stage.

Do we want to continue enforcing it?

If so, we should make sure all existing apps follow it.


Result

Yes, enforce model level filtering.

These apps needs to be fixed:

  • Backbone.js
  • Backbone.js + RequireJS
  • Backbone.Marionette
  • Agility.js
  • Dojo
  • Maria

Comment if you find more.

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 4, 2013

Contributor

What is it about the linked Spine model class that shows model-level filtering?

Contributor

petermichaux commented Jan 4, 2013

What is it about the linked Spine model class that shows model-level filtering?

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Jan 4, 2013

Member
Member

sindresorhus commented Jan 4, 2013

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 4, 2013

Contributor

I do see those methods.

My Maria example has similar methods in the Todos model yet my example was deemed not having model-level filtering. I'm confused.

Contributor

petermichaux commented Jan 4, 2013

I do see those methods.

My Maria example has similar methods in the Todos model yet my example was deemed not having model-level filtering. I'm confused.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Jan 4, 2013

Member

@petermichaux What methods? From what I can see you're just setting the mode in the model, while you should preferably have a couple of model methods to return a filtered model set as shown in the Spine model.

Member

sindresorhus commented Jan 4, 2013

@petermichaux What methods? From what I can see you're just setting the mode in the model, while you should preferably have a couple of model methods to return a filtered model set as shown in the Spine model.

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani Jan 4, 2013

Member

This one is tricky because although our specs may state that filtering should be done at a model level, we have to decide whether to enforce this rule when the 'common' way to approach this with a framework may be outside of a model itself (e.g within a View or Collection etc.). I agree however that some consistency where possible would be preferable.

For the Backbone app, I would be interested in hearing what @derickbailey @jsoverson etc. think about this. Guys, we're discussing filtering down Todo items based on state (e.g active, inactive, completed) but are wondering whether the filtering behavior (for Backbone) best lives inside the Model or outside of it. What do you think?

Member

addyosmani commented Jan 4, 2013

This one is tricky because although our specs may state that filtering should be done at a model level, we have to decide whether to enforce this rule when the 'common' way to approach this with a framework may be outside of a model itself (e.g within a View or Collection etc.). I agree however that some consistency where possible would be preferable.

For the Backbone app, I would be interested in hearing what @derickbailey @jsoverson etc. think about this. Guys, we're discussing filtering down Todo items based on state (e.g active, inactive, completed) but are wondering whether the filtering behavior (for Backbone) best lives inside the Model or outside of it. What do you think?

@derickbailey

This comment has been minimized.

Show comment Hide comment
@derickbailey

derickbailey Jan 4, 2013

Contributor

@addyosmani there's a re-opened issue on Marionette that talks about this: marionettejs/backbone.marionette#183

the gist of it is that i don't think filtering logic belongs in views. the view should just render the models that the collection hands to it.

Contributor

derickbailey commented Jan 4, 2013

@addyosmani there's a re-opened issue on Marionette that talks about this: marionettejs/backbone.marionette#183

the gist of it is that i don't think filtering logic belongs in views. the view should just render the models that the collection hands to it.

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 4, 2013

Contributor

The Backbone.js, Agility.js, and Dojo examples are using CSS to hide <li> elements in the list.

Contributor

petermichaux commented Jan 4, 2013

The Backbone.js, Agility.js, and Dojo examples are using CSS to hide <li> elements in the list.

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani Jan 4, 2013

Member

Thanks for chiming in @derickbailey :)

filtering logic belongs in views. the view should just render the models that the collection hands to it.

I agree with this. @sindresorhus how would you feel about us making this modification for the Backbone.js and Backbone.js + RequireJS apps for 1.0 (Maria.js if we can get it in) and address the rest of the modifications in 1.x?

Member

addyosmani commented Jan 4, 2013

Thanks for chiming in @derickbailey :)

filtering logic belongs in views. the view should just render the models that the collection hands to it.

I agree with this. @sindresorhus how would you feel about us making this modification for the Backbone.js and Backbone.js + RequireJS apps for 1.0 (Maria.js if we can get it in) and address the rest of the modifications in 1.x?

@jsoverson

This comment has been minimized.

Show comment Hide comment
@jsoverson

jsoverson Jan 4, 2013

Contributor

I agree, the filtering should be done by the time the models are set to be rendered.

The marionette+requirejs app (which uses CSS) was actually made for a training and some of the decisions stuck in the official Marionette version just due to time constraints.

Contributor

jsoverson commented Jan 4, 2013

I agree, the filtering should be done by the time the models are set to be rendered.

The marionette+requirejs app (which uses CSS) was actually made for a training and some of the decisions stuck in the official Marionette version just due to time constraints.

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 5, 2013

Contributor

I don't read CoffeeScript or Spine applications very well.

I see how the view is using getByFilter to populate the list of visible to-dos.

What I don't understand is the following. Suppose someone has the Completed filter clicked on. Then they create a new to-do. What is stopping that new to-do from appearing in the list of visible to-dos?

Contributor

petermichaux commented Jan 5, 2013

I don't read CoffeeScript or Spine applications very well.

I see how the view is using getByFilter to populate the list of visible to-dos.

What I don't understand is the following. Suppose someone has the Completed filter clicked on. Then they create a new to-do. What is stopping that new to-do from appearing in the list of visible to-dos?

@derickbailey

This comment has been minimized.

Show comment Hide comment
@derickbailey

derickbailey Jan 5, 2013

Contributor

@petermichaux i use this pattern to do my filtering on the collection: http://jsfiddle.net/derickbailey/7tvzF/ in the example you're describing i would have the filtered decorator listen to the "add" event of the original collection and re-run the filter when anything is added... or removed, for that matter ... not just on "reset" like the fiddle currently shows. this would prevent the new items from being shown in the list if they should not be shown.

Contributor

derickbailey commented Jan 5, 2013

@petermichaux i use this pattern to do my filtering on the collection: http://jsfiddle.net/derickbailey/7tvzF/ in the example you're describing i would have the filtered decorator listen to the "add" event of the original collection and re-run the filter when anything is added... or removed, for that matter ... not just on "reset" like the fiddle currently shows. this would prevent the new items from being shown in the list if they should not be shown.

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 5, 2013

Contributor

@derickbailey I agree that a decorator filter is a very good choice in this situation. The best choice, I think. This is why I created a decorator class for Maria set models: https://github.com/petermichaux/maria-SubsetModel/blob/master/src/SubsetModel.js

There are several parts of the view layer that should be watching different filtered sets of to-dos. The count of remaining to-dos would be a view observing the filtered set of active to-dos. The delete completed button would be observing the filtered set of completed to-dos. The control to mark all to-dos as complete and the input to create new to-dos would be observing unfiltered set of to-dos. The visible list is trickier because its model would change between the filtered active, filtered completed, and unfiltered set of to-dos depending on which filter the user has chosen.

Doing all this is how I've been wanting to write my example for Maria. Unfortunately, the multiple views and filtered models could make the Maria example seem more complex than the other examples. When someone reads the TodoMVC app code for all the various frameworks, the added size "to do things right" could make Maria seem comparatively difficult or labourious to use. The solution, however, would be more elegant.

I don't see any of the examples in TodoMVC using this decorator filter approach.

Contributor

petermichaux commented Jan 5, 2013

@derickbailey I agree that a decorator filter is a very good choice in this situation. The best choice, I think. This is why I created a decorator class for Maria set models: https://github.com/petermichaux/maria-SubsetModel/blob/master/src/SubsetModel.js

There are several parts of the view layer that should be watching different filtered sets of to-dos. The count of remaining to-dos would be a view observing the filtered set of active to-dos. The delete completed button would be observing the filtered set of completed to-dos. The control to mark all to-dos as complete and the input to create new to-dos would be observing unfiltered set of to-dos. The visible list is trickier because its model would change between the filtered active, filtered completed, and unfiltered set of to-dos depending on which filter the user has chosen.

Doing all this is how I've been wanting to write my example for Maria. Unfortunately, the multiple views and filtered models could make the Maria example seem more complex than the other examples. When someone reads the TodoMVC app code for all the various frameworks, the added size "to do things right" could make Maria seem comparatively difficult or labourious to use. The solution, however, would be more elegant.

I don't see any of the examples in TodoMVC using this decorator filter approach.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Jan 6, 2013

Member

I agree with this. @sindresorhus how would you feel about us making this modification for the Backbone.js and Backbone.js + RequireJS apps for 1.0 (Maria.js if we can get it in) and address the rest of the modifications in 1.x?

👍 (I guess you mean 1.1)

Unfortunately, the multiple views and filtered models could make the Maria example seem more complex than the other examples.

That's one of the downsides of using a simple todo app as an example. Doing it elegantly as you say would probably look a bit bloated on this app, but most likely scale much better on larger ones...

I of course still would prefer the elegant way. The trick is to explain why you did what you did in the readme. It's only bloat if the user doesn't know why it is.

Member

sindresorhus commented Jan 6, 2013

I agree with this. @sindresorhus how would you feel about us making this modification for the Backbone.js and Backbone.js + RequireJS apps for 1.0 (Maria.js if we can get it in) and address the rest of the modifications in 1.x?

👍 (I guess you mean 1.1)

Unfortunately, the multiple views and filtered models could make the Maria example seem more complex than the other examples.

That's one of the downsides of using a simple todo app as an example. Doing it elegantly as you say would probably look a bit bloated on this app, but most likely scale much better on larger ones...

I of course still would prefer the elegant way. The trick is to explain why you did what you did in the readme. It's only bloat if the user doesn't know why it is.

@petermichaux

This comment has been minimized.

Show comment Hide comment
@petermichaux

petermichaux Jan 7, 2013

Contributor

@sindresorhus, I'm still curious about this part...

I don't read CoffeeScript or Spine applications very well.

I see how the view is using getByFilter to populate the list of visible to-dos.

What I don't understand is the following. Suppose someone has the Completed filter clicked on. Then they create a new to-do. What is stopping that new to-do from appearing in the list of visible to-dos?

Contributor

petermichaux commented Jan 7, 2013

@sindresorhus, I'm still curious about this part...

I don't read CoffeeScript or Spine applications very well.

I see how the view is using getByFilter to populate the list of visible to-dos.

What I don't understand is the following. Suppose someone has the Completed filter clicked on. Then they create a new to-do. What is stopping that new to-do from appearing in the list of visible to-dos?

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Jan 10, 2013

Member

@petermichaux When a new todo is created the create and change events are dispatched. This executes addNew and because of a possible bug I had to set addAll to execute on change. This is stupid, since addAll will overwrite everything addNew did, but that's the best solution I found at that time to support routing, since addAll is filtered.

Should have done this originally, but at least created an issue for it #384 now.

Member

sindresorhus commented Jan 10, 2013

@petermichaux When a new todo is created the create and change events are dispatched. This executes addNew and because of a possible bug I had to set addAll to execute on change. This is stupid, since addAll will overwrite everything addNew did, but that's the best solution I found at that time to support routing, since addAll is filtered.

Should have done this originally, but at least created an issue for it #384 now.

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Jan 10, 2013

Member

Ok, so it looks like we agree the filtering logic should be in the model.

I've added a todo list in the issue description for which apps that needs to be fixed.

Member

sindresorhus commented Jan 10, 2013

Ok, so it looks like we agree the filtering logic should be in the model.

I've added a todo list in the issue description for which apps that needs to be fixed.

@ColinEberhardt

This comment has been minimized.

Show comment Hide comment
@ColinEberhardt

ColinEberhardt Feb 3, 2014

Member

This has been superseded by #790 which lists apps that filter in the view.

Member

ColinEberhardt commented Feb 3, 2014

This has been superseded by #790 which lists apps that filter in the view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment