Skip to content
This repository has been archived by the owner on Mar 20, 2021. It is now read-only.

Support for filter event (CollectionViewHelper) #351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support for filter event (CollectionViewHelper) #351

wants to merge 2 commits into from

Conversation

MichaReiser
Copy link
Contributor

According to the documentation (http://thoraxjs.org/api.html) it should be possible to trigger the filter event on a collection to enforce a refilter of a collection bound to a CollectionViewHelper (with an itemFilter).

Added handling of the filter event of a collection bound to a CollectionViewHelper

…ilter event on a collection to enforce a refilter of a collection bound to a CollectionViewHelper (with an itemFilter).

Added handling of the filter event of a collection bound to a CollectionViewHelper
@mulderp
Copy link

mulderp commented Apr 2, 2014

nice one!

… gets the parameters collection, response and options. The options parameter is not forwarded by thorax.

Forward the options parameter to the success handler.

http://backbonejs.org/#Collection-fetch
@kpdecker
Copy link
Contributor

@eastridge weren't we talking about not supporting this at one time in favor of calls on the collection view itself? My concern with this is you are emitting an event on a potentially shared object that may have only changed for one view.

@MichaReiser
Copy link
Contributor Author

What about only listening to the event if the collection view has set a filter? For all other instances it's not important anyway. Triggering it on the collection has the benefit, that it's not required to know the CollectionView instance, for example if the collection helper is used...

@kpdecker
Copy link
Contributor

That doesn't really impact the overhead, as applyVisibilityFilter does nothing if there is no itemFilter defined.

At a more conceptual level, this event is saying "hey collection, the view changed, you should tell the view that it changed", which feels wrong to me.

I've run into a number of scenarios where looking up the CollectionView instance was necessary, and somewhat painful to do so I wonder if it would be better to expose an API that makes that easier.

@eastridge
Copy link
Contributor

@kpdecker yes we did talk about that but it was never implemented. This PR makes the code more consistent with the docs but I agree that it's an abuse of what the collection object should be concerned with (using an event on the collection to force something to happen on the view)

@MichaReiser
Copy link
Contributor Author

I tested 3.x version with getCollectionViews(collection).
This way it's easy to update the view (call updateFilter). Maybe we could add a wrapper to getCollectionViews(collection) to return a composite that offers a updateFilter method with which all views can be updated (_.first(this.getCollectionViews(collection).updateFilter() is somehow complicated...).

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Micha Reiser seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants