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

includedByState filter not fully working #1735

Closed
cyrilf opened this issue Feb 5, 2015 · 7 comments · Fixed by #1736
Closed

includedByState filter not fully working #1735

cyrilf opened this issue Feb 5, 2015 · 7 comments · Fixed by #1736
Assignees

Comments

@cyrilf
Copy link
Contributor

cyrilf commented Feb 5, 2015

includedByState improvement proposition

tldr;

This works:

<div ng-show="'**.foo.**' | includedByState">Work</div>

and this don't.. (doesn't respect the second parameter, shown no matter what id is)

<div ng-show="'**.foo.**' | includedByState:{id: 'new'}">Do not work</div>

Long version

The downside with it is that it doesn't use all the params from the $state.includes function, it only cares about the first one, which is fullOrPartialStatename (as reported in the doc)

Here is a plunker to show it live. In this example you can see that the red message is shown, no matter the params passed to the filter.


The issue come from here in the code as it uses only a single param.

To resolve this, we can easily tweak the method to something like this: Gist, where params and options are given to the other method.

I may open a PR soon, but let me know what you think about it?

@eddiemonge
Copy link
Contributor

@cyrilf
Copy link
Contributor Author

cyrilf commented Aug 19, 2015

Hey @eddiemonge
I can understand that this isn't the most important issue. However, I think this is a quick win because I've already made the PR, and this isn't a breaking change. So I'll be very happy if you reconsider it.

(FYI, I've seen that the same issue will occurs in the 1.0 branch too, so I may push a PR soon if you want)

@eddiemonge
Copy link
Contributor

Keeping the PR open in case @christopherthielen wants to pull it in to 1.0

@christopherthielen
Copy link
Contributor

On mobile but this seems ok to me.

@christopherthielen
Copy link
Contributor

Link the PR?

@cyrilf
Copy link
Contributor Author

cyrilf commented Aug 20, 2015

The PR for this is here
I didn't took the time to make one for the 1.0, but I may do it soon.

@cyrilf
Copy link
Contributor Author

cyrilf commented Aug 20, 2015

Ok, so here you are for the PR to the feature-1.0

ExpFront pushed a commit to ExpFront/ui-router that referenced this issue Jun 23, 2016
Add parameters to the $IncludedByStateFilter:
- params
- options

This allows to use more parameters on this filter and not only the fullOrPartialStatename parameter.
Thanks to this improvement, the following now works (takes in account the id param):

<div ng-show="'**.foo.**' | includedByState:{id: 'new'}">It works</div>

Closes angular-ui#1735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants