Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No possibility to use multiple signal names #170

Closed
niieani opened this issue Dec 20, 2015 · 6 comments
Closed

No possibility to use multiple signal names #170

niieani opened this issue Dec 20, 2015 · 6 comments

Comments

@niieani
Copy link
Contributor

niieani commented Dec 20, 2015

@jdanyow asked for a use case, so here's an example:

HTML (task.html):

<div if.bind="tags | containsTag: 'test' ">
    (...)
</div>

There are two things that can trigger a change of the tags property in the view model - either the task gets changed (somebody adds or removes a tag in it's view-model) or somebody renames one of the existing tags globally. In both of those instances I would like the binding to get reevaluated, because its value might have changed. I can send out signals in both instances (i.e. a signal for the specific task changing it's tags and a global app signal for the changes in tags). Therefore I'd like to be able to specify multiple signal names, lets say something like:

<div if.bind="tags | containsTag: 'test' & signal: [ 'global-tags-renamed', 'task-change-' + id ] ">
    (...)
</div>

Now the binding should get reevaluated when either of those signals gets sent.

@EisenbergEffect
Copy link
Contributor

@jdanyow Couldn't the signal behavior be used multiple times?

@niieani
Copy link
Contributor Author

niieani commented Dec 20, 2015

@EisenbergEffect No, this gives an error A binding behavior named "signal" has already been applied.

@EisenbergEffect
Copy link
Contributor

@jdanyow Would it be possible to make multiple instances of signal possible? If not, could we enable the signal behavior to receive either a signal name or an array of signals? What are your thoughts?

@niieani
Copy link
Contributor Author

niieani commented Dec 20, 2015

I've made a non-breaking pull request that fixes this with rest parameters (all additional parameters are treated additional signal names). #172

@jdanyow
Copy link
Contributor

jdanyow commented Dec 20, 2015

@EisenbergEffect / @niieani - I was thinking the same as @niieani: update the behavior such that it accepts multiple name arguments.

@niieani: thanks for the PR! Could you please make a few changes so we can merge it:

  1. Please follow the commit message guidelines (in contributing.md). This enables us to automate the production of release notes among other things.
  2. Please change ...args to the old-school arguments style. This is a hot code path so we can't afford the overhead of the transpiled ...args code. Here's what I mean
  3. Please update the tests for the SignalBindingBehavior. The tests for the behaviors are kind of smelly, I can help with this if you get stuck or need a hand.

@niieani
Copy link
Contributor Author

niieani commented Dec 21, 2015

@jdanyow Sure, I'll do that when I get a moment.
@EisenbergEffect I was looking for some contributing guidelines (in many places, such as a link in the main README.md and in the Docs) before sending the PR, but I couldn't find anything aside from the information that signing the License is required. Perhaps it could be made clearer in the docs and/or the main readme that there is something like contributing.md and where to find it?

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

Successfully merging a pull request may close this issue.

3 participants