feat(uiStateActive): directive to add/remove classes for active state #635

caitp · 2013-12-05T03:47:26Z

Fixes #19

TODO: Still needs a test suite, I'll try to write some stuff tomorrow. In the mean time, here's a prototype.

Adds a directive to switch a set of classes in a DRY fashion by interacting with neighboring ui-sref directives.

The class to be used is an interpolated string, which is not $observed. There is no default, as I do not wish to tie this module to a particular CSS model or framework. A default may be configured in the future with a provider. As usual, multiple classes may be used, as Attributes.$addClass/$removeClass will separate classes on whitespace.

CAVEATS:

This is only capable of working as a parent of a ui-sref directive, or as a sibling directive on the same element. In the future, there may be a mechanism around this, but for the time being I can't think of what it would look like.

USAGE:

Sibling directive on same element as ui-sref:

<a ui-sref="foo.bar({baz: '123'})" ui-state-active="active" href>123</a>

--- OR ---

Ancestor directive of ui-sref:

<li class="item" ui-state-active="active">
 <a ui-sref="foo.bar({baz: '123'})" title="123" href>123</a>
</li>

nateabele · 2013-12-05T03:51:07Z

src/stateDirectives.js

+      }
+
+      // Test that we have the correct route params, if any
+      function matchingParams() {


What about angular.equals()?

well, lets try it --- I would have assumed that it would return false of o2 has a key that o1 doesn't, but this appears to be false...

...

seems to work in the plnk, I'll shrink that down a bit

nateabele · 2013-12-05T04:03:44Z

Hey, this is awesome. I'll write up some tests for ya tomorrow morning. Really the only other idea I had for this was the ability for the parent-style usage to respond to multiple uiSref children, which simplifies cases like menus, where you want the highlight to cascade. We can roll with this for now and get to that later though.

nateabele · 2013-12-05T04:19:08Z

src/stateDirectives.js

+      };
+
+      $scope.$on('$stateChangeSuccess', update);
+      $scope.$on('$routeChangeError', function() {


Okay, last thing, then I'll stop bothering you till tomorrow. 😄 Since ui.router doesn't broadcast any $route* events, I'm guessing you meant $stateChangeError?

Also, since a state change error just rolls back to the current state, I'm pretty sure only intercepting $stateChangeSuccess is enough.

er, yes... that is supposed to be $stateChangeError, hah.

Also, testing with http://jsbin.com/oKOZITa/1/edit it looks like angular.equals might not be a good solution (in case a route has parameters which are not taken into account by the ui-sref)

nateabele · 2013-12-05T14:22:12Z

Hey, I wrote up some tests. Just threw everything in a branch for convenience: https://github.com/angular-ui/ui-router/tree/caitp-issue-19

Feel free to keep working off the branch, or just squash it into your PR.

timkindberg · 2013-12-05T14:23:44Z

Awesome! Was also thinking it could be cool if you could use the directive to set other things other than a class, first one that came to mind was the title attr. Something like: ui-state-active='{class:"active", title:"You are here", foo:"bar"}'. Not sure there's enough "other" stuff to justify, at least at this point. But it was just an idea.

nateabele · 2013-12-05T14:25:32Z

It's easy to add later, but let's see if there's a use case for it. We also have filters now, which offer another approach to handling that.

timkindberg · 2013-12-05T14:29:32Z

Indeed

"Don't keep lists"
"Let your users be your memory"

caitp · 2013-12-05T14:33:47Z

Regarding the page title, suppose something like this, completely unrelated to the directive:

$stateProvider
  .state('app', {
    templateUrl: '...',
    controller: 'AppCtrl',
    abstract: true,
    url: '',
    title: 'ApplicationName'
  })
  .state('app.welcome', {
     ...,
     views: {
       'main@app': {
         title: 'Welcome'
       }
    });

The $state service could look at the state configuration and say "Okay, I guess I need the title to be ApplicationName - Welcome" or something.

I mean a directive might make it a bit more flexible, but it doesn't seem unreasonable to me to be able to do it that way

nateabele · 2013-12-05T14:40:03Z

Oh, I thought he meant the title attribute of the link. States already let you attach a hash of custom data which could be used for page titles.

caitp · 2013-12-05T14:48:05Z

Oh I see, yeah maybe.

@nateabele: your caitp-issue-19 branch has a lot of changes (compared with master), do you want to split some of that stuff out into smaller commits? Some of it looks like style fixes and some of it is new utility functions, etc

Adds a directive to switch a set of classes in a DRY fashion by interacting with neighboring ui-sref directives. The class to be used is an interpolated string, which is not $observed. There is no default, as I do not wish to tie this module to a particular CSS model or framework. A default may be configured in the future with a provider. As usual, multiple classes may be used, as Attributes.$addClass/$removeClass will separate classes on whitespace. CAVEATS: This is only capable of working as a parent of a ui-sref directive, or as a sibling directive on the same element. In the future, there may be a mechanism around this, but for the time being I can't think of what it would look like. USAGE: Sibling directive on same element as ui-sref: <a ui-sref="foo.bar({baz: '123'})" ui-state-active="active" href>123</a> --- OR --- Ancestor directive of ui-sref: <li class="item" ui-state-active="active"> <a ui-sref="foo.bar({baz: '123'})" title="123" href>123</a> </li>

nateabele · 2013-12-05T15:07:40Z

@caitp Yeah, sure. One of 'em is a utility function I copied over from viewpath, the others were just moved.

caitp · 2013-12-05T15:11:35Z

it might not be too big of a deal, it looks not too unclean this way

caitp · 2013-12-05T15:16:00Z

However, using @attrs.$add/$remove class is likely a better idea than $element, it's supposed to play nicer with ngAnimate + ngClass

nateabele · 2013-12-05T15:26:27Z

@caitp Yeah I wasn't sure about that one. I changed it because we still test against 1.0.6 (wherein those methods apparently don't exist), but I guess it's time to up the minimum requirements.

caitp · 2013-12-05T15:57:33Z

Since it's a bit tied to ui-sref, maybe ui-sref-active would be a more appropriate name as it does nothing if isolated from an sref

paramKeys aren't needed when using `equalForKeys()` strategy

timkindberg · 2013-12-05T16:20:03Z

maybe ui-sref-active would be a more appropriate name

I agree

caitp · 2013-12-05T17:44:58Z

@nateabele what would you prefer to do, merge the existing stuff and rename it before the next release, or something else?

nateabele · 2013-12-05T19:05:04Z

@caitp Yeah, I'm down with the rename. That way, we leave open the possibility of using uiStateActive for more advanced cases and/or cases disassociated from uiSref. I'd say, throw one more commit on the PR, and I'll go ahead and merge it.

nateabele · 2013-12-05T19:08:06Z

Anyway, after that I'll see about upgrading the minimum Angular version so we can go back to using the $attrs methods. @caitp @ProLoser Any advice or recommendations on which version(s) we should target?

caitp · 2013-12-05T19:13:22Z

you've already added support for the release candidate's updated ngAnimate stuff right?

Renamed to reflect the association with the ui-sref directive.

nateabele · 2013-12-05T19:24:08Z

On a branch, yeah, but I haven't done anything about ensuring those changes are compatible with < 1.2. Obviously the simplest thing would be to drop support for pre-1.2, but I haven't even upgraded my work apps to 1.2 yet.

feat(uiStateActive): directive to add/remove classes for active state

shql · 2013-12-05T23:34:26Z

Awesome. I also wrote something this week regarding #19. It's a slightly different approach to allow you setting a state name yourself if you do not have a "ui-sref" attribute "neighbour". I'll attach it in case someone finds it useful.

.directive('uiStateActive', function($state) {
    return {
        restrict: 'A',
        link: function (scope, element, attributes) {
            scope.$on('$stateChangeSuccess', function() {
                var stateName;

                if(typeof attributes.uiSref != 'undefined') {
                    stateName = attributes.uiSref;
                }

                if(typeof attributes.uiStateActive != 'undefined' && attributes.uiStateActive) {
                    stateName = attributes.uiStateActive;
                }

                if(stateName) {
                    if($state.includes(stateName)) {
                        element.addClass('active');
                    } else {
                        element.removeClass('active');
                    }
                }
            });
        }
    };
})

I wanted to finish this one first to make it more compatible and configurable. But as this is released, I'll just throw it in. 👍

nateabele · 2013-12-05T23:36:42Z

Thanks. Yeah, we'll probably roll with something similar later on.

chopachom · 2013-12-14T19:04:07Z

Would be nice if it would work with nested states, so that it would keep active class even if we navigate to substates

nateabele · 2013-12-14T19:39:48Z

@chopachom Yeah, there are plans for that. One thing at a time.

shql · 2013-12-15T00:54:16Z

Or take solutions like mine until then. :)

Am 14.12.2013 um 20:39 schrieb Nate Abele notifications@github.com:

@chopachom Yeah, there are plans for that. One thing at a time.

�
Reply to this email directly or view it on GitHub.

shirro · 2013-12-20T03:15:49Z

I have fairly high latency on my resolves as they go across the pacific.It is a bit of a UX failure pressing some nav element and waiting with no feedback. I wouldn't mind it if ui-sref-active listened on $stateChangeStart as well as $stateChangeSuccess and put a "loading" class on there while the resolves happen then removes it and adds the "active" class.

caitp · 2013-12-20T03:36:13Z

I'm not sure why doing that would need to be specific to this particular directive, but maybe (if it's not too large), we could have a separate directive doing that.

It seems like behaviour which is not totally part of the concerns of ui-sref-active. But I guess it all depends on what people think about it

timkindberg · 2013-12-22T17:04:03Z

I actually wanted a way to determine if a state was loading a long time ago, see #23. I agree with @caitp though. It is mostly unrelated to this. @shirro I'd say move any conversation about loading over to #23.

nateabele reviewed Dec 5, 2013
View reviewed changes

Caitlin Potter and others added 2 commits December 5, 2013 10:01

chore(uiStateActive): refactor & add test coverage

dde5d68

timkindberg mentioned this pull request Dec 5, 2013

docs for uiStateActive #639

Closed

chore(uiStateActive): remove unneeded code

13079ec

paramKeys aren't needed when using `equalForKeys()` strategy

chore(uiSrefActive): rename uiStateActive to uiSrefActive

4cea4ed

Renamed to reflect the association with the ui-sref directive.

nateabele added a commit that referenced this pull request Dec 5, 2013

Merge pull request #635 from caitp/issue-19

ae360fc

feat(uiStateActive): directive to add/remove classes for active state

nateabele merged commit ae360fc into angular-ui:master Dec 5, 2013

MrOrz mentioned this pull request Dec 19, 2013

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(uiStateActive): directive to add/remove classes for active state #635

feat(uiStateActive): directive to add/remove classes for active state #635

caitp commented Dec 5, 2013

nateabele Dec 5, 2013

caitp Dec 5, 2013

nateabele commented Dec 5, 2013

nateabele Dec 5, 2013

caitp Dec 5, 2013

nateabele commented Dec 5, 2013

timkindberg commented Dec 5, 2013

nateabele commented Dec 5, 2013

timkindberg commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

timkindberg commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

shql commented Dec 5, 2013

nateabele commented Dec 5, 2013

chopachom commented Dec 14, 2013

nateabele commented Dec 14, 2013

shql commented Dec 15, 2013

shirro commented Dec 20, 2013

caitp commented Dec 20, 2013

timkindberg commented Dec 22, 2013

feat(uiStateActive): directive to add/remove classes for active state #635

feat(uiStateActive): directive to add/remove classes for active state #635

Conversation

caitp commented Dec 5, 2013

nateabele Dec 5, 2013

Choose a reason for hiding this comment

caitp Dec 5, 2013

Choose a reason for hiding this comment

nateabele commented Dec 5, 2013

nateabele Dec 5, 2013

Choose a reason for hiding this comment

caitp Dec 5, 2013

Choose a reason for hiding this comment

nateabele commented Dec 5, 2013

timkindberg commented Dec 5, 2013

nateabele commented Dec 5, 2013

timkindberg commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

timkindberg commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

nateabele commented Dec 5, 2013

caitp commented Dec 5, 2013

nateabele commented Dec 5, 2013

shql commented Dec 5, 2013

nateabele commented Dec 5, 2013

chopachom commented Dec 14, 2013

nateabele commented Dec 14, 2013

shql commented Dec 15, 2013

shirro commented Dec 20, 2013

caitp commented Dec 20, 2013

timkindberg commented Dec 22, 2013