-
Notifications
You must be signed in to change notification settings - Fork 34
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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