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): new output that would notify when link is activated #43280
Conversation
@@ -72,6 +72,16 @@ import {RouterLink, RouterLinkWithHref} from './router_link'; | |||
* </div> | |||
* ``` | |||
* | |||
* You can use the output `isActiveChange` to get notified each time the link becomes |
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.
Let's move this documentation to be on the actual isActiveChange
output below. Also, you should mention what true
or false
indicates. I know it might seem implied already, but it's best to be explicit.
FYI, since I know there have been issues with the commit history in past PRs, you can use fixup commits that will be squashed when merged:
git commit --all --fixup HEAD
git push
https://github.com/angular/angular/blob/master/CONTRIBUTING.md#addressing-review-feedback
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.
LGTM 🍪
reviewed-for: public-api
Cool, so you still want me to make this documentation change right? |
That's right. Please still make the change. |
97a947e
to
31a1118
Compare
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.
reviewed-for: public-api
@atscott why did the test fail now? or is it an internal test? |
This commit adds a new output to `routerLinkActive` directive. Whenever, the associated link becomes active or inactive, an event will be fired on this out with the correct status PR Close angular#37284
31a1118
to
310aa33
Compare
@anandtiwary because we moved the documentation to the event, the public API removed the |
Thanks! |
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.
Reviewed-for: public-api
@atscott Is there an automated process that runs periodically and merges the approved PRs? just trying to understand the process. |
@anandtiwary There's no automated process. The caretaker periodically goes through the merge queue and merges things by hand. This will likely happen for this PR sometime tomorrow. |
@anandtiwary This has been merged into master. |
…gular#43280) This commit adds a new output to `routerLinkActive` directive. Whenever, the associated link becomes active or inactive, an event will be fired on this out with the correct status PR Close angular#37284 PR Close angular#43280
…gular#43280) This commit adds a new output to `routerLinkActive` directive. Whenever, the associated link becomes active or inactive, an event will be fired on this out with the correct status PR Close angular#37284 PR Close angular#43280
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit adds a new output to
routerLinkActive
directive.Whenever, the associated link becomes active or inactive, an
event will be fired on this out with the correct status
PR Close #37284
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #37284
What is the new behavior?
Does this PR introduce a breaking change?
Other information