Allow link-active to react to linkTo input changes #120
Conversation
|
All current tests pass, no linting errors. Didn't add tests for feature yet as am waiting to know if implementation is acceptable. |
|
A test has been added but it fails - as discussed in Gitter, I'd appreciate some help on why it fails. |
|
|
||
| constructor( | ||
| @Query(LinkTo) public links: QueryList<LinkTo>, | ||
| public element: ElementRef, | ||
| public router$: Router, | ||
| public renderer: Renderer | ||
| ) {} | ||
| ) { | ||
| this.links.changes.subscribe(_ => this.subscribeLinks()); |
There was a problem hiding this comment.
Move this out of the constructor into ngOnInit. We only want to use the constructor for wiring up dependencies
* master-upstream: docs(AngularCLI): Fixed quotes for map packages (#137) feat(LinkActive): Added override of default activeOptions for linkActive directive (#118) docs(AngularCLI): Separated docs and added required section about dependencies (#128) docs(Params) Added <string> type to pluck example (#129) docs(): Added section for router navigation and Angular CLI (#124) docs(SystemJS): Updated dependencies for SystemJS configuration (#122)
… link-active-detect-changes * origin/link-active-detect-changes: Simplified LinkTo event subscriptions Added test Allow link-active to react to linkTo input changes # Conflicts: # lib/link-active.ts
|
@brandonroberts the changes have been implemented, but the test is still not done correctly, and I would appreciate some help. Previously it failed because the @query subscriptions fired after the expectations ran. This was solved with fakeAsync, but now the issue is the Router is read directly (and not through the Observable), so doing router.next() doesn't work.. |
|
Using router.go() updates LocationStrategy, which makes the tests behave properly. Any reason for the spec to use router.next() that I might not be aware of? |
|
LGTM. Will you rebase on top of master? |
|
I believe I did, can you check? |
|
Merged. Thanks for your work and patience! |
|
Is this implemented in |
|
No. It will be released with the next beta.
|
Proposal to fix #119