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
docs: warn that directives don't support namespaces #25855
Conversation
You can preview c6c546b at https://pr25855-c6c546b.ngbuilds.io/. |
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.
Update git commit message to docs: warn that directives don't support namespaces
<!-- #enddocregion applied, --> | ||
<!-- #enddocregion applied --> | ||
|
||
<!-- #docregion unsupported --> |
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.
Is this needed even though you have this in a separate file?
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.
Sorry, what do you mean? I just followed the given examples 😄
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.
@s-gbz He means that you can remove lines 7-9 since you have this already in app.component.avoid.html
. Does that help?
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.
Yes, if im supposed to just delete lines 7-9 and push again 😀
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.
Yes, I think that's it. 🙂
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.
Thanks for this, @s-gbz! :) I have only one small comment.
df829da
to
f7eedb9
Compare
You can preview f7eedb9 at https://pr25855-f7eedb9.ngbuilds.io/. |
f7eedb9
to
22182a0
Compare
You can preview 22182a0 at https://pr25855-22182a0.ngbuilds.io/. |
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.
Though we generally try to avoid don't-do-this examples, this one seems reasonable.
@s-gbz will you remove the extra lines? Everything else LGTM. |
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.
title must be changed to header
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
You can preview 1d053e0 at https://pr25855-1d053e0.ngbuilds.io/. |
1d053e0
to
9dbce69
Compare
You can preview 9dbce69 at https://pr25855-9dbce69.ngbuilds.io/. |
Hello @brandonroberts, could we wrap this up real'quick? 😄 |
@s-gbz sure. Something has changed with your CLA status. Will you rebase on master and squash down to a single commit? |
9dbce69
to
426f147
Compare
CLAs look good, thanks! |
You can preview 426f147 at https://pr25855-426f147.ngbuilds.io/. |
@brandonroberts guess we did it then 🎉 |
426f147
to
2c0e5cc
Compare
@brandonroberts I just rebased the PR on current master *again. LGTM? |
You can preview 2c0e5cc at https://pr25855-2c0e5cc.ngbuilds.io/. |
Thanks for your work on this @s-gbz! Hopefully next time it won't take as long. |
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. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Augments current documentation.
What is the current behavior?
The issue has not been documented.
Issue Number: #24971
What is the new behavior?
Explicitly states that namespace are not supported in directives
and gives an example of how it's not done.
Does this PR introduce a breaking change?
Other information
Is this there anything to add @jenniferfell?