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

RFC: Nested state support for ui-sref-active #704

Closed
MrOrz opened this issue Dec 19, 2013 · 57 comments
Closed

RFC: Nested state support for ui-sref-active #704

MrOrz opened this issue Dec 19, 2013 · 57 comments
Labels
Milestone

Comments

@MrOrz
Copy link

MrOrz commented Dec 19, 2013

It is mentioned in #635 and #19 that ui-sref-active will support nested state in the future, but I could not find the details discussed anywhere. As I browse the current source code, I found that it is actually not hard to achieve, and that it would seem redundant if I create another directive just for supporting nested states.

Since I need this feature right away, I've implemented a quick and dirty way to meet my need: when an extra html attribute ui-nested is present, ui-sref-active would use $state.includes instead of strict equality on the states.

Demo: http://plnkr.co/edit/CWUy3T0M7dTap75hASkk?p=preview

This can be achieved by merely changing 1 line (yeah I'm only capable of making a modification <= 4 lines... :P)
https:/MrOrz/ui-router/compare/ui-nested

Hope that through this little dirty hack, we can initiate a discussion on how this function should be properly implemented.

@timkindberg
Copy link
Contributor

Nice I've at least triaged it and added to Milestone 0.4.0. Thanks.

@hazeledmands
Copy link
Contributor

What about a separate directive called ui-sref-nested that adds relevant classes if the referenced state includes the current state? Seems like it would be useful to have the ability to set different CSS in either case.

@MrOrz
Copy link
Author

MrOrz commented Dec 25, 2013

@demands Wow I haven't thought of that. It surely may be useful to apply different classes to separate the cases that a state directly active from activation of descendant states.

For example, with a separate directive ui-sref-nested we can achieve something like this:

<div ui-sref-nested="is-child-selected" ui-sref-active="is-selected"></div>

@timkindberg
Copy link
Contributor

I kind of like this functionality but not necessarily the solution. It feels a tad odd for an official solution. Talking about @demands idea.

@hazeledmands
Copy link
Contributor

What would be better?

@timkindberg
Copy link
Contributor

Perhaps its the naming that I don't like, I see the need for the two solutions. ui-sref-nested feels a bit ambiguous, like is it talking about nested srefs or sref-active's? Obviously I know what its doing I'm just thinking of a typical user. There's so much to take in already in ui-router I'm just trying to play the friendly-api police and devils advocate.

Like maybe another good idea is to have only the ui-sref-active directive but it behaves differently in different circumstances, using the example ui-sref-active="selected":

  • If the ui-sref on the same element or a child is active it adds the class "selected"
  • if the ui-sref that is active is nested then it adds the classes "selected selected-nested" or maybe "selected selected-child", not sure which would be a better name.
  • Maybe in all cases we also add the class "sref-active"?

@MrOrz
Copy link
Author

MrOrz commented Jan 3, 2014

@timkindberg the *-nested solution sounds great!
sref-active class sounds cool too. However I am not sure about its use case.

The only scenario I came up with is to apply additional styles if there is an element with ui-sref attribute.

For example:

<li ui-sref-active="...">
  <a ui-sref="..." ng-if="!disabled">Step 3</a>
  <span ng-if="disabled">Step 3</span>
</li>

And <li> would have sref-active if and only if scope.disabled is false.

@timkindberg
Copy link
Contributor

@MrOrz cool thanks. Yeah the idea for adding 'sref-active' in all cases was so you'd always have a common class among all active srefs regardless of what class name you specified, but I think its probably not so useful. If @nateabele is ok with the idea of appending the extra *-nested class in those special cases, I'd say you are good to go on submitting a PR.

@besnik81
Copy link

besnik81 commented Jan 8, 2014

A very quick hack i've used for recursively activating the parents. I m almost sure that it is not a correct implementation, but it should give an idea:

https:/besnik81/ui-router/compare/patch-1

It works in very well in my case, which is similar to the following snippet.

  <ul>
    <li ui-sref-active="active">
         <a href="#">Parent</a>
         <ul>
            <li ui-sref-active="active">
               <a href="#" ui-sref="my.state">Child</a>
            </li>
         </ul>
    </li>
  </ul>

@MrOrz
Copy link
Author

MrOrz commented Jan 8, 2014

@besnik81 The idea that one ui-sref can toggle the activation state of two nested ui-sref-active is really interesting! In my current implementation of ui-sref-active it requires an ui-sref to tell it what state to look at.

If the ui-sref-active cannot be recursively (in terms of HTML structure) activated, the only way to achieve something similar is to add something dirty to our HTML:

  <ul>
    <li ui-sref-active="active">
         <a ui-sref="my" style="display: none;"></a> Parent
         <ul>
            <li ui-sref-active="active">
               <a href="#" ui-sref="my.state">Child</a>
            </li>
         </ul>
    </li>
  </ul>

Not to mention that the solution I provided above cannot deal with the case where inner <ul> contains different parent states:

  <ul>
    <li ui-sref-active="active">
         <a ui-sref="??? Dunno which to choose ???" style="display: none;"></a> Parent
         <ul>
            <li ui-sref-active="active">
               <a href="#" ui-sref="my.state">Child</a>
            </li>
            <li ui-sref-active="active">
               <a href="#" ui-sref="your.state">Child</a>
            </li>
         </ul>
    </li>
  </ul>

So there are 2 "nested ui-sref-active" patterns we currently have in this thread:

  1. The "states" are nested, as in http://plnkr.co/edit/CWUy3T0M7dTap75hASkk?p=preview .
  2. The HTML strucutre are nested. as in the snippet provided by @besnik81 .

And these two patterns are not necessarily independent! Thing's getting complicated now...... :/

@timkindberg
Copy link
Contributor

I don't fully understand how the ideas differ

@besnik81
Copy link

besnik81 commented Jan 9, 2014

@timkindberg The essential difference is that the state hierarchy does not necessarily correspond to the HTML hierarchy. The code I've provided focuses on activating parent nodes in the HTML, whenever a link inside that node is activated, regardless of the active state. My idea is that we need these additional classes to support CSS styling -- which in turn is using only the HTML structure. The primary use will be for styling menus. I think we can have multiple not-related states included under the same menu and still wanting to activate that menu item.

The solution given by @MrOrz focuses on activating links that correspond to parent states (hope I am right here).

Definitely, there are two patterns here.

@besnik81
Copy link

besnik81 commented Jan 9, 2014

To clear the ideas (or just thinking loud):

First, ui-sref will transition into a state (say app.users.list that lists the users). Clicking on a user, ui-sref="app.users.view(34)", will transition into a new child state.

  • Should the link to app.users.list be activated? I think that the choice should be left to the user. In this case, we need an attribute (say ui-sref-active) with a pattern of states to activate the item. This attribute should not be linked to that particular ui-sref but rather listen to state change events. The good thing is that it will be completely decoupled from ui-sref. The bad thing can be in terms of performance.
  • How to specify the classes? Maybe a new attribute like ui-sref-active-class that will only add/toggle classes. This attribute will be linked with all children ui-sref-active attributes -- same as the actual relationship between ui-sref and ui-sref-active?!
  • How to propagate activation up the DOM tree? We can link a ui-sref-active to all ui-sref-active-class attrs above. But, how to order the deactivation from a child with the activation from another child. Need to see if there are different events for 'before state change' and 'after state change'.

@timkindberg
Copy link
Contributor

The solution given by @MrOrz focuses on activating links that correspond to parent states (hope I am right here).

@besnik81 ah I don't think I ever picked up on the fact that is was based off of the actual state nesting, I always was thinking in terms of how the ui-sref-active directive would interact with ui-srefs nested in the html, regardless of which states the ui-srefs pointed. I do understand now, at least how there are more things to consider, but I'm a bit lost as to how to approach it. Sounds like it's getting overly complex to me at this point. Its fine to get complex while hashing things out and discussion but we'll need to steer this way back to super simple at the end; I mean it is just a directive for highlighting something, so it needs to be dead easy. Also I'd prefer dead easy and hit 80% of use cases, rather than complex and hit 100%.

@ericclemmons
Copy link

I have a multi-level navigation that uses nested states extensively, and this PR would solve several of the UI issues I'm having.

I'll have to catch up on this thread & grok the "2 patterns" so hopefully we can reach a conclusion :)

@MrOrz
Copy link
Author

MrOrz commented Jan 10, 2014

I came up with an idea during shower :P

It combines the HTML hierarchy (using scope events) and state hierarchy alltogether.

Let's say we have an HTML element with ui-sref-active="is-selected" and there are some elements with ui-sref as its descendent.

  1. All ui-sref-active classes are removed at the beginning of $stateChangeSuccess events.
  2. During the $stateChangeSuccess event, ui-sref uses $scope.$emit to bubble an event of its state & params being the current state. Let's name the event $uiSrefSelected for convenience.
  3. ui-sref-active adds the class is-selected upon receiving $uiSrefSelected event.
  4. During the $stateChangeSuccess event, ui-sref uses $scope.$emit to bubble another event when one of its child states being active. Let's name the event $uiChildSrefSelected for convenience.
  5. ui-sref-active adds the class is-selected-nested upon receiving $uiChildSrefSelected event.
  6. It's possible that both is-selected and is-selected-nested are applied to an element at the same time, since an element with ui-sref-active may contain multiple elements with different ui-sref. This case can be easily styled using CSS selectors.

In this way the ui-sref-active would work as the original, while it provides more flexibility because ui-sref-active now does not need to be a direct parent of ui-sref.

Is there any unsatisfactory scenario I have not thought of?

@timkindberg
Copy link
Contributor

@MrOrz very well thought out. One potentially missing scenario (not sure how important it is though, keeping in my the 80/20 rule). A scenario like this where the nested ui-sref point to non-nested states (but really just any nested ui-sref regardless of its state target):

<ul>
    <li ui-sref="stateA" ui-sref-active>
        <ul>
            <li ui-sref="stateB"/>
        </ul>
    </li>
</ul>

We'd have to have a 3rd css class I think: $uiNestedSrefSelected. It's probably not a likely case at least for at first though. So I like your solution @MrOrz!

Also I would prefer the following naming conventions:

  • $uiSrefActivated instead of $uiSrefSelected
  • $uiSrefChildStateActivated instead of $uiChildSrefSelected
  • $uiNestedSrefActivated as this potential 3rd class, again not sure if its needed.

@gfpacheco
Copy link

Hi guys, while you think about which one is the perfect solution, can't you please implement the @MrOrz 4 lines solution?
I would like to use this feature and his solution would work for most cases.

@nateabele
Copy link
Contributor

@gfpacheco Nope, so it's a good thing it's so easy for you to roll your own module while you wait.

@hazeledmands
Copy link
Contributor

@gfpacheco I rolled my own janky solution not too long ago. Check out this gist if you'd like to copy it.

@gfpacheco
Copy link

Yes it's easy, but I'm using bower to manage 3rd paty scripts. Well, I'll uninstall it and hard code it, thanks.

@MrOrz
Copy link
Author

MrOrz commented Jan 16, 2014

Sorry I am currently on other projects and didn't have time for a PR.

FYI you can put file path in bower.json so that when you bower install it fetches the code directly from a local file. Bower is innocent, don't kill it!

For example, a line from my bower.json just reads:

"angular-ui-router": "/Users/mrorz/workspace/ui-router/build/angular-ui-router.js"

@timkindberg
Copy link
Contributor

I'm fine with @MrOrz last solution, If anyone wants to submit a PR.

@mlegenhausen
Copy link

+1

@MrOrz
Copy link
Author

MrOrz commented Jan 25, 2014

Upon seeing #818 and #819 I decided to push myself to implement what we have discussed in this thread.

The event names are after @timkindberg 's suggestions.

The forked branch I am currently working at: https:/MrOrz/ui-router/compare/704

Plunker example: http://plnkr.co/edit/YJ35VMz5AjP1nVIGvNKV?p=preview

Some additional effort is made to work out the situation when an URL of a certain state is directly navigated, since AngularJS seems to instantiate ui-sref prior to ui-sref-active.

Now when the following URL is directly navigated, "Bob" should be surrounded in red borders and "Users" should have pink background.
http://run.plnkr.co/WTbX28dpesnmrMWk/#/users/users/Bob

I am going to add some tests, taking #819 as a precious reference.

For the first time in forever, I will be writing a test! ❄️

@MrOrz
Copy link
Author

MrOrz commented Jan 25, 2014

This is a plunker of @besnik81 's snippet with the implementation applied.

http://plnkr.co/edit/LJlUnSfdUVyTQLQxThzv?p=preview

Notice that since the parent ui-sref-active relies on ui-sref to tell it if the class is-selected should be added, in the arrangement of this snippet, the class applied to the outmost ui-sref-active will be is-selected, rather than is-selected-nested.

MrOrz added a commit to MrOrz/ui-router that referenced this issue Jan 25, 2014
…f-active

uiSrefActive now takes all decendant uiSrefs into consideration.
uiSrefActive also adds a class with "-nested" postfix when a child state of the related uiSrefs becomes active.

Closes angular-ui#704.
@besnik81
Copy link

Sorry for this late reply.

@MrOrz, if you would like to have a look at this implementation ().

https:/besnik81/ui-router/compare/patch-2

I did this 2 weeks ago, but apparently haven't posted correctly my comment about this. It recursively asigns classes to all parent ui-sref-active. It also supports a ui-sref-active="is-active|is-parent-active" syntax, where the second class is added only when it is indirectly activated. I am using an already existing event to clear the classes "(no need for new events). I think it does not break existing behavior.

@MrOrz
Copy link
Author

MrOrz commented Feb 12, 2014

@besnik81 Are you planning to send a pull request with your implementation in the future?
I really like your implementation and am willing to provide help on sending a pull request if needed. :)

@lanceschi
Copy link

@sjdweb: I've tried your approach, albeit unsuccessfully. Where do you put state.includes('root.parent')? In which controller?

Is the #819 already available in the latest master branch? Which syntax? I've read it, altough I haven't found any evidence.

Thanks,
Luca

@sjdweb
Copy link

sjdweb commented Feb 20, 2014

@lanceschi
To make it work you need to make $state available in your view.

angular.module('xyz').controller('AbcController', ['$scope', '$state', function($scope, $state) {
   $scope.$state = $state;
}]);

@lanceschi
Copy link

@sjdweb: thanks! It did the trick! Nice workaround while waiting for the official implementation.

@jkarttunen
Copy link

Thanks @sjdweb, just what i was looking for.

@timkindberg
Copy link
Contributor

Check out #927 for latest progress on this

@jhnns
Copy link

jhnns commented Mar 7, 2014

When will this be released?

@timkindberg
Copy link
Contributor

In the next release most likely... so within 4 weeks?

@jhnns
Copy link

jhnns commented Mar 7, 2014

Thx, looking forward to. 👍

@jkarttunen
Copy link

My workaround:
elem ng-class="{ active: $state.includes('root.*')}"

(Also how can i use html code in commments withtout it getting parsed as html?

@MrOrz
Copy link
Author

MrOrz commented Mar 13, 2014

@jkarttunen try Fenced code blocks ?

@stryju
Copy link

stryju commented Mar 14, 2014

👏 perfect - exactly what i was looking for :-)

rebuilding locally worked out nicely

@corydorning
Copy link

what's the suggested implementation for this? seen several linked threads but not an official stance on how to implement it.

@dmackerman
Copy link

@jkarttunen: your solution doesn't work for me. 😦

@Grsmto
Copy link

Grsmto commented May 15, 2014

@dmackerman this is probably because you didn't define $state in your view $scope : $scope.$state = $state;

@dmackerman
Copy link

@Grsmto: I actually have it on my $rootScope - so that should work, no?

$rootScope.$state = $state;
$rootScope.$stateParams = $stateParams;

@Grsmto
Copy link

Grsmto commented May 15, 2014

@dmackerman yes I suppose. I just tried it right at the time I wrote my comment and it works...

@sentient
Copy link

@jkarttunen works for me as well in order to show a tab active for any child state.

One note to others. This is a replacement of ui-sref-active attribute.
Don't specify both mechanisms

@christopherthielen christopherthielen modified the milestones: 0.2.11, 1.5.0 Nov 16, 2014
@atesgoral
Copy link

You can also use the includedByState filter if you don't want to include $state in the scope:

ng-class="{ active: ('root.*' | includedByState) }"

@chbrown
Copy link

chbrown commented Feb 6, 2015

Just to clarify to anyone coming to this thread late, like me, from an old-ish version of ui-router: the feature described in this issue is now implemented in ui-router, at least in the current version (0.2.13).

The old ui-sref-active behavior is replaced by ui-sref-active-eq, and the new ui-sref-active applies its values as class names whenever the corresponding ui-sref is a ancestor of the current route.

I use a hierarchical ui-router paradigm in my app, with lots of abstract states, so I have a nav bar that looks like this:

<nav>
  <span ui-sref="admin.administrators" ui-sref-active="current" class="tab">
    <a ui-sref="admin.administrators.table">Administrators</a>
  </span>
  <span ui-sref="admin.experiments" ui-sref-active="current" class="tab">
    <a ui-sref="admin.experiments.table">Experiments</a>
  </span>
  <!-- other links ... -->
</nav>
<ui-view></ui-view>

The nav's children, the spans, are not actual links (and those span's ui-sref states are abstract) but I want them to activate whether the user is on the .table or .new, or any other subroutes. Not entirely DRY, but it works!

@lekhnath
Copy link

@atesgoral 's solution worked for me. short and sweet, would like to thank for letting know about that.

@atesgoral
Copy link

@lekhnath You're welcome :)

@stryju
Copy link

stryju commented May 31, 2015

the fix is in place, tho

@lekhnath
Copy link

lekhnath commented Jun 1, 2015

I was actually trying to add an active class in ionicframework's ion-item directive and don't want to add span and anchors for just a purpose of adding active class, so for me atesgoral solution was best suit. If the same requirement came in web development then I should go with the newer (fix) approach.

@arepalli-praveenkumar
Copy link

@atesgoral Please provide a working example
Thanks for your help...

@arepalli-praveenkumar
Copy link

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

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

No branches or pull requests