-
Notifications
You must be signed in to change notification settings - Fork 6.8k
docs: Show directive selector in generated API docs #4139
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
// Categorize the current visited classDoc into its Angular type. | ||
if (isDirective(classDoc)) { | ||
classDoc.isDirective = true; | ||
classDoc.directiveExportAs = getDirectiveExportAs(classDoc); | ||
classDoc.selectors = getDirectoveSelectors(classDoc); |
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.
directiveSelectors
?
'textarea[md-autosize]', | ||
'[overlay-origin]', | ||
'[connected-overlay]', | ||
]; |
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.
I think this should be a constant at the top of the 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.
Good idea. I had thought of that, but decided to go with this since it seemed specific to this function. I'll make these changes once I get the CLA figured out.
if (selectorMatches) { | ||
selectorMatches = selectorMatches.split(/\s*,\s*/); | ||
selectorMatches = selectorMatches | ||
.filter(match => match !== '' && !match.includes('mat') && blacklist.indexOf(match) === -1); |
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.
How about the following?
return selectorMatches.split(/\s*,\s*/)
.filter(match => match && !match.includes('mat') && !blacklist.includes(match));
.filter(match => match !== '' && !match.includes('mat') && blacklist.indexOf(match) === -1); | ||
} | ||
|
||
return selectorMatches; |
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.
If you immediately return (as with comment above) then you can just remove that.
CLAs look good, thanks! |
Update from original
@@ -1,3 +1,11 @@ | |||
const DIRECTIVE_SELECTOR_BLACKLIST = [ |
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.
Add a comment like
/**
* We want to avoid emitting selectors that are deprecated but don't have a way to mark
* them as such in the source code. Thus, we maintain a separate blacklist of selectors
* that should not be emitted in the documentation.
*/
'[portalHost]', | ||
'textarea[md-autosize]', | ||
'[overlay-origin]', | ||
'[connected-overlay]', |
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.
You can make this a Set
instead of an array:
const SELECTOR_BLACKLIST = new Set([
'[portal]',
'[portalHost]',
'textarea[md-autosize]',
'[overlay-origin]',
'[connected-overlay]',
]);
And then use SELECTOR_BLACKLIST.has(...)
instead of includes
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.
Taken care of. Let me know if it's all up to par. Also, thanks for the tip.
|
||
return selectorMatches ? selectorMatches.split(/\s*,\s*/) | ||
.filter(match => match !== '' && !match.includes('mat') && !DIRECTIVE_SELECTOR_BLACKLIST.includes(match)) | ||
: selectorMatches; |
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.
I'd shorten match
to just s
so that it's
return selectorMatches ?
selectorMatches.split(/\s*,\s*/)
.filter(s => s && !s.includes('mat') && !SELECTOR_BLACKLIST.has(s))
: selectorMatches;
@@ -3,14 +3,21 @@ <h4 class="docs-api-h4 docs-api-class-name"> | |||
</h4> | |||
<p class="docs-api-class-description">{$ class.description $}</p> | |||
|
|||
{%- if class.directiveSelectors -%} | |||
<span class="docs-api-h4 docs-api-class-selector-label">Selectors:</span> |
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.
Selectors:
-> selector:
I'd like it to match what's in the source
@@ -3,14 +3,21 @@ <h4 class="docs-api-h4 docs-api-class-name"> | |||
</h4> | |||
<p class="docs-api-class-description">{$ class.description $}</p> | |||
|
|||
{%- if class.directiveSelectors -%} | |||
<span class="docs-api-h4 docs-api-class-selector-label">Selectors:</span> |
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.
Can you wrap the inside of this if block in a `<div class="docs-api-directive-selectors>"?
@@ -3,14 +3,21 @@ <h4 class="docs-api-h4 docs-api-class-name"> | |||
</h4> | |||
<p class="docs-api-class-description">{$ class.description $}</p> | |||
|
|||
{%- if class.directiveSelectors -%} | |||
<span class="docs-api-h4 docs-api-class-selector-label">Selectors:</span> |
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.
This isn't an <h4>
so you should omit docs-api-h4
@@ -3,14 +3,23 @@ <h4 class="docs-api-h4 docs-api-class-name"> | |||
</h4> | |||
<p class="docs-api-class-description">{$ class.description $}</p> | |||
|
|||
{%- if class.directiveSelectors -%} | |||
<span class="docs-api-class-selector-label">selector:</span> | |||
<div class="docs-api-directive-selectors"> |
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.
This div should be around the selector-label
span as well
* Adds method to get directive selectors to categorizor.js * Adds selectors to class.template.html Takes care of #3983 Conflicts: tools/dgeni/templates/class.template.html
LGTM |
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. |
Takes care of #3983