search

rotundasoftware / backbone.collectionView

Easily render backbone.js collections. In addition to managing model views, this class supports automatic selection of models in response to clicks, reordering models via drag and drop, and more.
http://rotundasoftware.github.io/backbone.collectionView/
Other
171 stars 27 forks source link

Ensure subviews of the collectionview stop listening to events when they... #31

Closed dwt closed 11 years ago

dwt commented 11 years ago

... get removed from the baby sitter. See rotundasoftware/backbone.collectionView#30 .

dgbeck commented 11 years ago

Hi @dwt ! Thanks very much for this PR! Very nice and clean.

There is something that seems semantically off to me to have model views stop listening when the collectionView's stopListening method is called. Maybe its because the model views may have a totally separate area of events that they are interested in... It seems possible that there would be a case when you want the collectionView to stop listening to it's events but you don't want the model views to stop listening to theirs.

I don't have that same feeling about the model views being removed when the collection view's "remove" method is called though. That seems semantically ok, since there is no way you would want to remove the collectionView without removing the models. Would that work for your use case as well? That is, an addition of basically what you have here but using on the remove method instead of the stopListening method ?

rbu commented 11 years ago

Forwarding stopListening only on remove would map to our use case of the collectionView just fine. Personally, I don't see the case where you'd not want the collection to update any more but still want elements inside it to react to events (yet), and if it is needed, the caller can do collectionView.stopListening(collectionView.collection) unless the model views are listening to the collection model as well. However, I'm open to both solutions, whichever gets merged faster ;-)

dgbeck commented 11 years ago

Ok, let's do the remove approach if that works for you. We can change this for you after the merge or you are welcome to work this in. Either way we will need a new PR to the dev branch, as there are some other pending changes, and then we'll merge to master and do a version bump ASAP.

Thanks very much for the important feedback and contribution.

dgbeck commented 11 years ago

Let's get this done as well @go-oleg. THank you!

go-oleg commented 11 years ago

Merged into master as part of f51a157039c150f45591e464b9a6f9b6154c5d5d.

Thanks!