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(router): RouterActive #6407

Closed
wants to merge 1 commit into from
Closed

Conversation

PatrickJS
Copy link
Member

a common use case for css frameworks is having the active class on the <li> rather than the <a> tag

@PatrickJS
Copy link
Member Author

idk why one test is failing :/ https://travis-ci.org/angular/angular/jobs/101662320

@petebacondarwin petebacondarwin added feature Issue that requests a new feature comp: router and removed comp: router labels Jan 29, 2016
@petebacondarwin petebacondarwin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 7, 2016
@petebacondarwin
Copy link
Member

@gdi2290 I think that being able to do this is an important use case.

The way this directive is implemented right now is too convoluted. (I appreciate that there were not so many options at the time of writing). It should not really be relying upon a ngLink directive being there; and especially not on its CSS class.

I would even suggest that we don't really need a directive for this. If we had a navigation component, which is responsible for rendering the navigation links, then it could expose a method called isActive(linkParams). We could then use this method with ngClass to get the required behaviour, quite cleanly, right?

<ul>
  <li [ngClass]="{active: isActive(['SomeRoute'])}"><a [ngLink]="['SomeRoute']>Some Route</a></li>
  ...
</ul>

Or you could even ditch ngClass and write to the class directly:

<ul>
  <li [class.active]="isActive(['SomeRoute'])"><a [ngLink]="['SomeRoute']>Some Route</a></li>
  ...
</ul>

@petebacondarwin petebacondarwin added action: discuss and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 29, 2016
@choeller
Copy link
Contributor

@petebacondarwin Why do you think the directive is too convoluted? I think the scenario is really common and having the possibilty too do:

<li router-active="active">
  <a [routerLink]="['/This', 'Is', 'My' ,'Nested', 'Route']">My Link</a>
</li>

is way more convenient then doing:

<li [class.active]="isActive([''/This', 'Is', 'My' ,'Nested', 'Route''])">
  <a [routerLink]="['/This', 'Is', 'My' ,'Nested', 'Route']">My Link</a>
</li>

@gdi2290 I tried to use your code-block in my project and found a small bug in your commit - maybe this is also why the checks fail. Here:

  constructor(router: Router, element: ElementRef, renderer: Renderer,
               @Query(RouterLink) routerLink: QueryList<RouterLink>,
               @Attribute('router-active') routerActiveAttr: string) {
              ....
   }

you are passing routerActiveAttr but never use it afterwards. Just out of curiosity: Why do you make a distinction beetween attribute and input-binding? I solved it like this in my version and I think its working fine:

@Directive({selector: '[router-active]', inputs: ['routerActive: router-active']})
export class RouterActive {
  routerActive: string = 'active';
  constructor(router: Router, element: ElementRef, renderer: Renderer,
              @Query(RouterLink) routerLink: QueryList<RouterLink>) {
    router.subscribe(() => {
      let active = routerLink.first.isRouteActive;
      renderer.setElementClass(element.nativeElement, this.routerActive, active);
    });
  }
}

Is there an advantage of making a distinction?

@zoechi
Copy link
Contributor

zoechi commented Mar 18, 2016

The selector should be routerActive instead of router-active since templates are case sensitive.

@Query(RouterLink) routerLink: QueryList<RouterLink>,
@Attribute('router-active') routerActiveAttr: string) {
router.subscribe(() => {
let active = routerLink.first.isRouteActive;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the RouterActive @directive idea, but my only concern is that when I have a child route then in this case the child route will be active as well which I not expect:

/home (if this is active)
/home/users (then let active = routerLink.first.isRouteActive; is also will be active)

Would not be better to use similar line to the following during set the active value:
let active = StringUtil.startsWith(_location.path(), _routerLink.first.visibleHref);
// StringUtil.startsWith(str, prefix)

Thanks for your feedback @gdi2290

@mhevery
Copy link
Contributor

mhevery commented May 20, 2016

This PR is stale (does not rebase; pass CI; had no activity for a while). It is probably our fault, and we are sorry. In an effort to keep our PR queue under control, and prevent us from taking too long to get back to you we are cleaning up the queue. Unfortunately this means that this PR is getting closed.

If you would like to resurrect this change, please rebase on master, make the tests pass and create a new PR.

@mhevery mhevery closed this May 20, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: discuss cla: yes feature Issue that requests a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants