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

Allow link-active to react to linkTo input changes #120

Merged
merged 9 commits into from
Jun 8, 2016
39 changes: 34 additions & 5 deletions lib/link-active.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
ElementRef,
Input,
OnDestroy,
OnInit,
Query,
QueryList,
Renderer,
Expand All @@ -13,6 +14,8 @@ import {
} from '@angular/core';
import { LinkTo } from './link-to';
import { Router } from './router';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/operator/mergeAll';

export interface LinkActiveOptions {
exact: boolean;
Expand All @@ -33,11 +36,12 @@ export const LINK_ACTIVE_OPTIONS: LinkActiveOptions = {
* </ol>
*/
@Directive({ selector: '[linkActive]' })
export class LinkActive implements AfterViewInit, OnDestroy {
export class LinkActive implements AfterViewInit, OnInit, OnDestroy {
@Input('linkActive') activeClass: string = 'active';
@Input() activeOptions: LinkActiveOptions;
private _sub: any;
private _activeOptions: LinkActiveOptions = { exact: true };
private _routerSub: any;
private _linksSub: any;

constructor(
@Query(LinkTo) public links: QueryList<LinkTo>,
Expand All @@ -49,14 +53,20 @@ export const LINK_ACTIVE_OPTIONS: LinkActiveOptions = {
private defaultActiveOptions: LinkActiveOptions
) {}

ngOnInit () {
this.links.changes.subscribe(_ => {
this.subscribeLinks();
});
}

ngAfterViewInit() {
if (this.defaultActiveOptions && !this.activeOptions) {
this._activeOptions = this.defaultActiveOptions;
} else if (this.activeOptions) {
this._activeOptions = this.activeOptions;
}

this._sub = this.router$
this._routerSub = this.router$
.map(({path}) => this.router$.prepareExternalUrl(path || '/'))
.subscribe(path => {
this.checkActive(path);
Expand All @@ -80,9 +90,28 @@ export const LINK_ACTIVE_OPTIONS: LinkActiveOptions = {
});
}

subscribeLinks() {
Copy link
Member

Choose a reason for hiding this comment

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

Managing way too many subscriptions here. Make more use of rx and merge the observables together into a stream you can subscribe to instead of looping through each linkTo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes added below comment

if (this._linksSub) {
this._linksSub.unsubscribe();
}

let observables = this.links.map(link => {
return link.hrefUpdated;
});

this._linksSub = Observable.from(observables)
.mergeAll()
.subscribe(_ => {
this.checkActive(this.router$.prepareExternalUrl(this.router$.path() || '/'));
});
}

ngOnDestroy() {
if (this._sub) {
this._sub.unsubscribe();
if (this._routerSub) {
this._routerSub.unsubscribe();
}
if (this._linksSub) {
this._linksSub.unsubscribe();
}
}
}
5 changes: 4 additions & 1 deletion lib/link-to.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Directive, HostBinding, HostListener, Input } from '@angular/core';
import { Directive, HostBinding, HostListener, Input, Output, EventEmitter } from '@angular/core';
import { Router } from './router';

/**
Expand All @@ -25,6 +25,8 @@ export class LinkTo {
this._updateHref();
}

@Output() hrefUpdated: EventEmitter<string> = new EventEmitter<string>();

private _href: string;
private _query: string | Object;

Expand All @@ -47,6 +49,7 @@ export class LinkTo {
let path = this._cleanUpHref(this._href);

this.linkHref = this._router.prepareExternalUrl(path, this._query);
this.hrefUpdated.emit(this.linkHref);
}

/**
Expand Down
39 changes: 37 additions & 2 deletions spec/link-active.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
iit,
async,
inject,
expect
expect,
fakeAsync,
tick
} from '@angular/core/testing';
import { MockLocationStrategy, SpyLocation } from '@angular/common/testing';
import { TestComponentBuilder } from '@angular/compiler/testing';
Expand All @@ -24,7 +26,10 @@ import { ROUTER_PROVIDERS, Router } from '../lib/router';
template: '',
directives: [LinkTo, LinkActive]
})
class TestComponent {}
class TestComponent {
public link1: string;
public link2: string;
}

const compile = (tcb: TestComponentBuilder, template: string = '') => {
return tcb
Expand Down Expand Up @@ -73,6 +78,36 @@ describe('Link Active', () => {
});
})));

it('should react to changes to the linkTo parameter', fakeAsync(inject([TestComponentBuilder, Router], (tcb, router$) => {
router$.go('/page');

return compile(tcb, `
<a id="page" linkActive="active" [linkTo]="link1">Page</a>
<a id="other" linkActive="active" [linkTo]="link2">Other</a>`)
.then((fixture) => {
let component = fixture.componentInstance as TestComponent;
let compiled = fixture.debugElement.nativeElement;
let linkPage: Element = compiled.querySelector('a#page');
let linkOther: Element = compiled.querySelector('a#other');

component.link1 = '/page';
component.link2 = '/other';
fixture.detectChanges();
tick();

expect(linkPage.getAttribute('class')).toEqual('active');
expect(linkOther.getAttribute('class')).not.toEqual('active');

component.link1 = '/other';
component.link2 = '/page';
fixture.detectChanges();
tick();

expect(linkPage.getAttribute('class')).not.toEqual('active');
expect(linkOther.getAttribute('class')).toEqual('active');
});
})));

it('should support multiple classes on the active element', async(inject([TestComponentBuilder, Router], (tcb, router$) => {
router$.next({
path: '/page'
Expand Down