fix(uiSrefActive): optionally match child states

[caitp]

This issue provides support for optionally counting a link as active if any of
its child states are active and parameters match.

Closes #818

[nateabele]

I dig it. Agreed re: attribute name, though. What about ui-inherit or ui-cascade?

[caitp]

I think something descriptive would be good, but I'm not totally sure what that would be. cascade might make sense but only if you really understand the state machine (so maybe bad for designers).

I dunno, should take a vote on it or something

[timkindberg]

@caitp @nateabele I really like the syntax and approach we've come up with over at #704, namely this comment: #704 (comment). @MrOrz has also written a PR, so we need to come together on the implementation. @caitp can you review #704 and see what you think?

[caitp]

@timkindberg I don't really care for the implementation in 821, it drastically changes how the machinery works and introduces a lot more than just what is asked for in 704/818. I think we're better off sticking to the simplest possible implementation. I don't believe there is much use in adding yet another state-change event to the library, or changing the way ui-sref itself works.

Having said that, @nateabele is the one maintaining this repository so he's the one who is going to decide which approach to go with.

[MrOrz]

@caitp The discussion in #704 started as a RFC for naming things as well. In the original post I only modified 4 lines of code. @timkindberg mentioned that we could achieve the same functionality without introducing a new vocabulary to keep using uiSrefActive really simple.

Later on, @besnik81 provided another use case in this comment. From that comment I realized that we can push uiSrefActive a little further to make it more flexible. We can support both nested "states" and nested uiSrefActives for multi-level menus, yet still not introducing a new attribute vocabulary to ui-router.

As for the implementation, I agreed that using angular events is probably an overkill. However, @besnik81 proposed a simpler way to achieve this with recursively activating uiSrefActive.

Please take a look at besnik's implementation: https://github.com/besnik81/ui-router/compare/patch-2

I did not actually run the code but I think it can support nested state with some additional if-statements added in checkNotifySrefActive.

Hope we can soon find a way to balance between implementation and usability :)

[nateabele]

Alright, let's take a vote on the attribute name and wrap this up. I propose ui-inherit or ui-cascade. /cc @angular-ui.

[MrOrz]

@timkindberg Thank you so much for the discussion we've made in #704 . I am sorry that the proposal in #704 is not currently adopted due to my immature implementation. I also agree that we should adopt a quick solution to the simple nesting problem for now. I guess I will submit another pull request some other day when #704 is implemented in a preferred way like @besnik81 's implementation.

[nateabele]

@MrOrz It's two different nesting problems. We'll come back to the original problem proposed by #704 when someone has time to do a clean implementation.

[timkindberg]

let's take a vote on the attribute name and wrap this up

@nateabele ok. I don't like ui-inherit or ui-cascade, because there are too many concepts for inherit and without namespacing the directive a little better they say nothing about being tied to ui-sref-active.

I actually like as-parent better, but again it needs name spacing IMO. But this is too cumbersome:

<a ui-sref="contacts" ui-sref-active="active" ui-sref-active-as-parent>Contacts</a>

I'd rather just see it as a second different (but similar) directive, like so:

<a ui-sref="contacts" ui-sref-active-parent="active">Contacts</a>

This seems the most intuitive to me from a API perspective.

[mogelbrod]

I think @timkindberg has a great idea, which would allow items to be marked as active, active+parent or parent with the following markup:

<a ui-sref="contacts" ui-sref-active="active" ui-sref-active-parent="parent">Contacts</a>

[lanceschi]

Hi, which is the approved syntax then? Thanks, L.

[timkindberg]

@caitp you like ui-sref-active-parent as a separate directive, instead of using as-parent as a modifier for ui-sref-active?

[caitp]

Whatever you guys want to do, I'm not terribly invested in this --- if someone wants to send a PR to make changes to this patch, or create their own version, or just amend it to behave slightly differently, I'm cool with any of these.

[bvaughn]

+1 for the ui-sref-active and ui-sref-active-parent suggestion.

[davemerwin]

+1 for the ui-sref-active and ui-sref-active-parent suggestion.

[timkindberg]

Someone want to pick up where @caitp left off?

[timkindberg]

I'm picking this up. I was thinking instead of ui-sref-active and ui-sref-active-parent we instead alter ui-sref-active so that it activates when any child state is active (because I think this is typically how devs will intend it to work) and then add a new one called ui-sref-active-is (or ui-sref-active-equals) which would match only the specific state and no children. Anyone have any thoughts on that?

Also I ran into an annoyance when trying to update the sample. The Contacts button should be active when any subsection is active, but its not going to work because clicking contacts takes you to contacts.list, but all the other states are inside contacts.detail which is NOT a child. In this case I need the ui-sref-active to be active on any children of 'contacts' which is an abstract state on top of all the contact states. So I have a problem where the active needs to activate on a different state than the ui-sref targets... I'll try to think of a way to deal with this... or it may be out of scope.

[MrOrz]

+1 for @timkindberg 's suggestion on the new ui-sref-active and ui-sref-active-equals.

IMHO as for the contacts button sample, an workaround is to support class activation via DOM hierarchy, which was proposed by @besnik81 in this comment in #704, if the Contacts button is the parent of all subsections. (Actually I cannot find the Contacts button sample Tim mentioned 😕 )

[timkindberg]

@MrOrz here's the sample link: http://angular-ui.github.io/ui-router/sample/#/

[timkindberg]

Or maybe we should name them ui-sref-active-is and ui-sref-active-includes to indicate the type of check being made in the code ($state.is and $state.includes respectively). That would be a breaking change but also very clear.

[timkindberg]

Or maybe we should just turn on inherited activation and just only have ui-sref-active... I mean give me an example of when you wouldn't want it on.

[nateabele]

Those really seem unnecessarily verbose. Can't we just keep the existing structure and find some good short names for the directive and the parent/not-parent flag?

[timkindberg]

@nateabele yeah I agree, I'm just gonna do ui-sref-active and ui-sref-active-equals. ui-sref-active will now match on all child states as well, and if you don't want that, just type '-equals', bam done.

[davemerwin]

+1 @timkindberg Great idea. Seems more useful. Otherwise how would you create exceptions?

[timkindberg]

Check out #927 for latest progress on this.

[MrOrz]

@timkindberg got the sample :)
The Contact button case is tricky, since the button's state is not a ancestor of activated state, and the button element is not an ancestor of the activated directives (so the DOM hierarchy activation doesn't help either). I came up with something like ui-sref-active="active@contact" or ui-sref-active="active on contact", but to think the class activation for a specific state can be achieved by ng-class within 10LOC, maybe it is ok if we put this off for now.

[timkindberg]

yeah I'm gonna just consider that particular use case out of scope and I just used ng-class on the sample.

[byeval]

i prefer the ui-sref-active-includes

[arepalli-praveenkumar]

@caitp Please provide working example

[arepalli-praveenkumar]

http://plnkr.co/edit/V67mToDSN0SbzaGJTHw2?p=preview
Here is working Example