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

feat(uiSrefActive): Also activate for child states. #927

Merged
merged 1 commit into from Mar 12, 2014

Conversation

Projects
None yet
@timkindberg
Copy link
Contributor

commented Mar 4, 2014

feat(uiSrefActive): Also activate for child states.

To limit activation to target state use new ui-sref-active-equals directive'

Breaking Change: Since ui-sref-active now activates even when child states are active you may need to swap out your ui-sref-active with ui-sref-active-equals, thought typically we think devs want the auto inheritance.

Fixes #818

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2014

I actually still want to add the 'active-child' class too if its a child state that is active, so this is not fully ready yet. This picks up where #819 and #821 left off. Should be done by tomorrow.

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2014

Actually no I don't, I don't want to add bloat for no reason which I think I've been guilty of a little bit lately when helping people brainstorm. Let's just get this in and then if we find that people want a special class for when the directive is activated by a child state we'll add it in a new commit.

@nateabele, @MrOrz, please review this PR and let's merge it.

@MrOrz

This comment has been minimized.

Copy link

commented Mar 5, 2014

👍 good to go!

@spaceribs

This comment has been minimized.

Copy link

commented Mar 5, 2014

I get: "TypeError: Cannot read property 'name' of null" occuring on line 248 if you have an angular binding as the ui-sref:

<a ui-sref="{{ nav.name }}">{{ nav.label }}</a>

We should probably have a test in there for that.

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2014

@spaceribs ok thanks, will add that.

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2014

@spaceribs actually there are no tests for that functionality, I'm not sure it was ever expressly supported. Have you ever used interpolation in a ui-sref successfully?

@nateabele

This comment has been minimized.

Copy link
Member

commented Mar 6, 2014

@spaceribs Yeah, that's not a thing.

@timkindberg I dig the functionality, but the directive name is still too verbose for me. What about this?

<a ui-sref="state" ui-sref-active="foo" ui-active-child="bar">...</a>
@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2014

Well it would have to be named something else because ui-sref-active activates on child so the other one doesn't. I'll think some more too.

@spaceribs

This comment has been minimized.

Copy link

commented Mar 6, 2014

@nateabele @timkindberg I was able to use it before without an error, and it worked fine. Why wouldn't you want to be able to dynamically set the sref? is there a better way?

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2014

@spaceribs its not that I wouldn't want it, its just that it wasn't technically a supported feature and is out of scope for this issue. You can open a new feature request issue once this one is merged though.

@spaceribs

This comment has been minimized.

Copy link

commented Mar 6, 2014

@timkindberg Alright, thanks and sorry for throwing it in to this PR, kinda surprised it wasn't one of the tests already.

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2014

@nateabele so I think it still needs to say ui-sref-active in the name because its literally the same thing. How about ui-sref-active-is?

feat(uiSrefActive): Also activate for child states.
To limit activation to target state use new `ui-sref-active-eq` directive

Breaking Change: Since ui-sref-active now activates even when child states are active you may need to swap out your ui-sref-active with ui-sref-active-eq, thought typically we think devs want the auto inheritance.

Fixes #818
@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2014

@nateabele Ok I shortened it to ui-sref-active-eq. Can we meet in the middle on that name? I think it important to maintain the 'ui-sref-active' portion of the name.

@treymack

This comment has been minimized.

Copy link

commented Mar 10, 2014

Ya. Googlability.
On Mar 9, 2014 8:26 PM, "Tim Kindberg" notifications@github.com wrote:

@nateabele https://github.com/nateabele Ok I shortened it to
ui-sref-active-eq. Can we meet in the middle on that name? I think it
important to maintain the 'ui-sref-active' portion of the name.

Reply to this email directly or view it on GitHubhttps://github.com//pull/927#issuecomment-37145298
.

@MrOrz

This comment has been minimized.

Copy link

commented Mar 10, 2014

👍 for keeping ui-sref-active- prefix

nateabele added a commit that referenced this pull request Mar 12, 2014

Merge pull request #927 from angular-ui/issue-818
feat(uiSrefActive): Also activate for child states.

@nateabele nateabele merged commit 5a7c48c into master Mar 12, 2014

1 check passed

default The Travis CI build passed
Details

@nateabele nateabele deleted the issue-818 branch Mar 12, 2014

@techniq

This comment has been minimized.

Copy link

commented Mar 13, 2014

I saw 0.2.10 was just released, which didn't include this change. Any idea how long till 0.2.11 is tagged? I tried to bower install sha #5a7c48ca08 but ran into some issues.

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2014

@techniq ouch, yeah... so releases have typically been taking a month. You could cherry pick and build locally.

@techniq

This comment has been minimized.

Copy link

commented Mar 13, 2014

@timkindberg - thanks. might just be an issue with my build system. I'll try to re-pull again. I kind of figured the release window was around then, which is why I asked.

@techniq

This comment has been minimized.

Copy link

commented Mar 14, 2014

@timkindberg - I figured out why I wasn't seeing the change even after specifying angular-ui-router's version in package.json to the SHA #5a7c48ca08. I'm using the gulp plugin gulp-bower-files which reads each bower_component's bower.json's main property, and I take each of those scripts and concat / minify. angular-ui-router's main is set to ./release/angular-ui-router.js, which is only rebuild for each release.

@techniq

This comment has been minimized.

Copy link

commented Mar 14, 2014

@timkindberg - FYI, I got around the main issue by setting an override in my bower.json for angular-ui. For example

    "overrides": {
        "angular-ui-router": {
            "main": "src/*.js"
        }
    }
@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2014

@techniq ah cool thanks.

@bpampuch

This comment has been minimized.

Copy link

commented on bf163ad Apr 19, 2014

why this change didn't get into released version (0.2.10)

This comment has been minimized.

Copy link
Member

replied Apr 19, 2014

...Because it didn't exist yet?

This comment has been minimized.

Copy link

replied Apr 19, 2014

oh, looks like a good reason :) any plans for 0.2.11?

This comment has been minimized.

Copy link
Member

replied Apr 19, 2014

The next version will probably be 0.3, released in the next 2-3 weeks.

This comment has been minimized.

Copy link
Contributor Author

replied Apr 20, 2014

Uhm it's in the changelog though for 0.2.10.

This comment has been minimized.

Copy link

replied Jul 10, 2014

+1

This comment has been minimized.

Copy link

replied Jul 16, 2014

+1

This comment has been minimized.

Copy link

replied Aug 28, 2014

I think you shouldn't limit by using the first only "nearest" uiSrefActive directive but all the parents. It will be helpful to mark as "active" not the selected menu only but the root menu(s) as well. Also - why not to set "active" as default class name? Or/and make a provider to allow user to define the default active class-name.

This comment has been minimized.

Copy link
Contributor Author

replied Aug 29, 2014

This has been released!!

This comment has been minimized.

Copy link

replied Jan 18, 2015

@timkindberg
I'm not so sure you got me:

  1. It would be helpful to mark the menu item as "active" as well, but this will not work:
el = angular.element('<li dropdown ui-sref-active="active"><a dropdown-toggle>Menu</a><ul class="dropdown-menu"><li><a ui-sref="contacts.item({ id: 1 })" ui-sref-active="active">Contacts</a></li></ul></li>');
  1. The active class is "" by default:
    Line 3583: activeClass = $interpolate($attrs.uiSrefActiveEq || $attrs.uiSrefActive || '', false)($scope);
    But why not simplify:
el = angular.element('<li dropdown ui-sref-active><a dropdown-toggle>Menu</a><ul class="dropdown-menu"><li><a ui-sref="contacts.item({ id: 1 })" ui-sref-active>Contacts</a></li></ul></li>');

?

@corydorning

This comment has been minimized.

Copy link

commented May 8, 2014

There are several issues opened for this functionality. Which one should I be following?

Also what's the proper implementation and timeline for release? Thanks!

@thillai

This comment has been minimized.

Copy link

commented May 29, 2014

@nateabele when can we expect version 0.3 with this fix?

@manuel-woelker

This comment has been minimized.

Copy link

commented Jul 17, 2014

+1. Jumping on the bandwagon and eagerly awaiting the next release

@gregwym

This comment has been minimized.

Copy link

commented Jul 24, 2014

+1. Really need this feature. Looking forward to new release.

@vik-singh

This comment has been minimized.

Copy link

commented Jul 24, 2014

+1

3 similar comments
@tjshipe

This comment has been minimized.

Copy link

commented Jul 28, 2014

+1

@island205

This comment has been minimized.

Copy link

commented Jul 29, 2014

+1

@lrodziewicz

This comment has been minimized.

Copy link

commented Jul 29, 2014

+1

@xuhong

This comment has been minimized.

Copy link

commented Jul 30, 2014

+1024

@derek-watson

This comment has been minimized.

Copy link

commented Jul 30, 2014

image

@srph

This comment has been minimized.

Copy link

commented Jul 31, 2014

+1 👍

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2014

Do you guys want this feature?

@sjmorrow

This comment has been minimized.

Copy link

commented Jul 31, 2014

+(everyone who uses ui-router)

@xuhong

This comment has been minimized.

Copy link

commented Jul 31, 2014

@timkindberg yes! i really don't want to use ng-class and $state.includes() to obtain the class.

@gregwym

This comment has been minimized.

Copy link

commented Aug 1, 2014

+10086
The 0.3.0 milestone is past due by 3 month. lol.

@quanghoc

This comment has been minimized.

Copy link

commented Aug 1, 2014

+1

1 similar comment
@mike-marcacci

This comment has been minimized.

Copy link

commented Aug 5, 2014

+1

@mike-marcacci

This comment has been minimized.

Copy link

commented Aug 5, 2014

@timkindberg does this PR still fix this issue? I can't get it to work in my builds, and it doesn't look like the test (at least from the PR's commit) actually tests successful activation of when on a child route. Perhaps I'm missing something?

@ghost

This comment has been minimized.

Copy link

commented Aug 10, 2014

+1

@petrkotek

This comment has been minimized.

Copy link

commented Aug 10, 2014

As there is still no official build including this functionality, I forked ui-router and did own release in my fork: https://github.com/bc-petrkotek/ui-router/tree/0.2.11-beta

You can do the same or use my one by adding following line in your bower.json:

"angular-ui-router": "git@github.com:bc-petrkotek/ui-router.git#0.2.11-beta".

@Fingel

This comment has been minimized.

Copy link

commented Aug 12, 2014

Wasted a bunch of time assuming this was the default implementation to begin with...
+1

@timkindberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

@WD-42 sorry man

@mike-marcacci actually I think we realized this only activates for the current and immediate child ui-sref. We'll have to add another PR for all descendant ui-srefs... :(

@kaiku

This comment has been minimized.

Copy link

commented Aug 28, 2014

Great work on ui-router, a big +1 for this useful feature

@nerdburn

This comment has been minimized.

Copy link

commented Aug 29, 2014

Awesome, this works for me in 0.2.11, thanks.

@bpampuch

This comment has been minimized.

Copy link

commented on bf163ad Apr 19, 2014

why this change didn't get into released version (0.2.10)

This comment has been minimized.

Copy link
Member

replied Apr 19, 2014

...Because it didn't exist yet?

This comment has been minimized.

Copy link

replied Apr 19, 2014

oh, looks like a good reason :) any plans for 0.2.11?

This comment has been minimized.

Copy link
Member

replied Apr 19, 2014

The next version will probably be 0.3, released in the next 2-3 weeks.

This comment has been minimized.

Copy link
Contributor Author

replied Apr 20, 2014

Uhm it's in the changelog though for 0.2.10.

This comment has been minimized.

Copy link

replied Jul 10, 2014

+1

This comment has been minimized.

Copy link

replied Jul 16, 2014

+1

This comment has been minimized.

Copy link

replied Aug 28, 2014

I think you shouldn't limit by using the first only "nearest" uiSrefActive directive but all the parents. It will be helpful to mark as "active" not the selected menu only but the root menu(s) as well. Also - why not to set "active" as default class name? Or/and make a provider to allow user to define the default active class-name.

This comment has been minimized.

Copy link
Contributor Author

replied Aug 29, 2014

This has been released!!

This comment has been minimized.

Copy link

replied Jan 18, 2015

@timkindberg
I'm not so sure you got me:

  1. It would be helpful to mark the menu item as "active" as well, but this will not work:
el = angular.element('<li dropdown ui-sref-active="active"><a dropdown-toggle>Menu</a><ul class="dropdown-menu"><li><a ui-sref="contacts.item({ id: 1 })" ui-sref-active="active">Contacts</a></li></ul></li>');
  1. The active class is "" by default:
    Line 3583: activeClass = $interpolate($attrs.uiSrefActiveEq || $attrs.uiSrefActive || '', false)($scope);
    But why not simplify:
el = angular.element('<li dropdown ui-sref-active><a dropdown-toggle>Menu</a><ul class="dropdown-menu"><li><a ui-sref="contacts.item({ id: 1 })" ui-sref-active>Contacts</a></li></ul></li>');

?

@fxck

This comment has been minimized.

Copy link

commented Feb 6, 2015

Is it gonna cover this case?

I have a link in menu that points to app.something.list(), but I want it to be active even when I'm on app.something.detail().. app.something is an abstract state.

@maciej-gurban

This comment has been minimized.

Copy link

commented Mar 27, 2015

+1 for what @fxck suggests.

@wcandillon

This comment has been minimized.

Copy link

commented Mar 27, 2015

@fxck @maciej-gurban I've made a proposal for this at #1254 What do you think?

@fxck

This comment has been minimized.

Copy link

commented Mar 27, 2015

I'd say this should be opt out rather than opt in, and if opt in, then not by explicitly naming the states. What you proposed is basically the same as using ngclass..

@uguryilmaz

This comment has been minimized.

Copy link

commented May 22, 2015

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.