Fix docs usage section for directives with void attributes #16265
Conversation
Narretz
changed the title
| {%- if doc.restrict.attribute -%} | ||
| <li>as attribute: | ||
| {% code %} | ||
| <{$ doc.element $} | ||
| {%- for param in doc.params %} | ||
| <{$ doc.element $} {%- if not hasNameAsParam %} |
There was a problem hiding this comment.
given that you have used {% - to ignore the whitespace, you may as well start this if block on the next line.
There was a problem hiding this comment.
Something like:
<{$ doc.element $}
{%- if not hasNameAsParam %}
{$ lib.directiveParam(param.name, param.type, '="', '"') $}
{% endif %}
{%- for param in doc.params %}
{$ lib.directiveParam(param.name, param.type, '="', '"') $}
{%- endfor %}>
There was a problem hiding this comment.
Thanks - I still need to include {%- endif -%} because otherwise there's a blank line after the directive name when hasNameAsParam is true.
When a directive doesn't take a value, its name is not included in the parameters, which previously meant that the directive name was missing from the Attribute / CSS Class usage section of the docs. This commit adds the name to the Usage section when it is missing from the parameters. Closes angular#14045
Narretz
force-pushed
the
docs-usage
branch
2 times, most recently
from
950ad1b to
314c8b9
Compare
|
Added tests to ensure that the directive name always appears in the usage section, whether it appears in the params or not. |
There was a problem hiding this comment.
This looks good. Thanks for the tests. I think they could be clearer if you did:
a) used helpers to find the params and usage elements, to hide the complexity of the CSS mangling from the tests
b) restructured the descriptions of the tests. Perhaps something like:
directive API docs
when directive name is a param with a value
should show directive name in params
should show directive name in usage
when directive name is a void param
should not show directive name in params
should show directive name in usage
|
Thanks for the review. I introduced helpers. |
There was a problem hiding this comment.
Excellent! Let's get this in...
One minor nit (but not blocking you merging this):
If one of the tests fails it will look like:
directives parameter section when directive name is a void param should not show the directive name in the parameters.
This doesn't read that well to me. I would drop the inner describe blocks altogether and just have longer it clauses. E.g.
describe('directives', () => {
describe('parameter section', () => {
it('should show the directive name only if it is a named param', () => {
browser.get('build/docs/index.html#!/api/ng/directive/ngInclude');
expect(getParamNames().getText()).toContain('ngInclude | src');
browser.get('build/docs/index.html#!/api/ngRoute/directive/ngView');
expect(getParamNames().getText()).not.toContain('ngView');
});
Then you would get an error message like:
directives parameter section should show the directive name only if it is a named param
etc
|
Don't forget to squash the extra commit :-) |
When a directive can be used as an attribute or CSS class, but doesn't take a value, its name is not included in the parameters, which previously meant that the directive name was missing from the Attribute / CSS Class usage section of the docs. This commit adds the name to the Usage section when it is missing from the parameters. Closes #14045 Closes #16265
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Improvement for the docs app
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: