Skip to content
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

build(docs-infra): improve directive API doc templates #25768

Closed

Conversation

petebacondarwin
Copy link
Member

Closes #22790
Closes #25530

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours severity1: confusing target: patch This PR is targeted for the next patch release labels Aug 31, 2018
@petebacondarwin petebacondarwin added this to the Backlog milestone Aug 31, 2018
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Aug 31, 2018
@petebacondarwin petebacondarwin added this to REVIEW in docs-infra Aug 31, 2018
@mary-poppins
Copy link

You can preview 3db2590 at https://pr25768-3db2590.ngbuilds.io/.

@jenniferfell
Copy link
Contributor

notes from mtg 9/6:
selectors: no table. no indent. UL. 1 LI per selector.
all tables: no code inside table, very long description or code link to "Usage notes" section
move inputs and outputs into properties table?
idea: separate these?

  • properties
  • inherited properties
  • methods
  • inherited methods (inherited from x)
    idea: remove class sig from top - everything is described below (for directives)
    idea: "selector usage" section instead of generated selector, hand-craft content instead
    idea: exports as - "template variable references" is ok. change var to mytemplatevar or myvarname.

@mary-poppins
Copy link

You can preview d5b4625 at https://pr25768-d5b4625.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 06bdf9b at https://pr25768-06bdf9b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 029962b at https://pr25768-029962b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 0763676 at https://pr25768-0763676.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 42fd7de at https://pr25768-42fd7de.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 876f92f at https://pr25768-876f92f.ngbuilds.io/.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random issue:
When a member gets no link (due to disambiguation issues???), then it looks different than the rest (example):

animationmetadatatype-rendering-issue

@@ -28,12 +28,14 @@
{%- endmacro -%}

{%- macro renderMemberSyntax(member, truncateLines) -%}
{%- if member.boundTo %}<span class="property-binding">@{$ member.boundTo.type $}({$ member.boundTo.bindingName $})</span>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the weird indentation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we need a new line after the @Input(...) text. But I think I can use <br> to get the same effect...

@@ -6,5 +6,5 @@
{% if doc.isAbstract %}abstract {% endif%}class {$ doc.name $}{$ doc.typeParams | escape $}{$ memberHelper.renderHeritage(doc) $} {{$ memberHelper.renderMembers(doc) $}
}
</code-example>
{$ descendants.renderDescendants(doc, 'class', 'Subclasses') $}
{$ descendants.renderDescendants(doc, 'class', 'Subclasses', true, r/class|directive|pipe|decorator/) $}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about component?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no components in the angular/angular repo.

@@ -6,5 +6,5 @@
}
</code-example>
{$ descendants.renderDescendants(doc, 'interface', 'Child interfaces') $}
{$ descendants.renderDescendants(doc, 'class', 'Class implementations') $}
{$ descendants.renderDescendants(doc, 'class', 'Class implementations', true, r/class|directive|pipe|decorator/) $}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about component?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

</li>
{% endfor %}
</ul>
{% endif %}
{% endmacro -%}

{%- macro renderDescendants(doc, docType, title='', recursed=true) %}
{% set descendants = doc.descendants | filterByPropertyValue('docType', docType) %}
{%- macro renderDescendants(doc, descendantType, title='', recursed=true, docTypeMatcher = descendantType) %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent whitespace around = 😁

const value = obj[propertySegments[index]];
return index < (propertySegments.length - 1) ? readProperty(value, propertySegments, index + 1) : value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be safer to support not having an intermediate object at all. E.g.: readProperty({a: {}}, ['a', 'b', 'c'], 0)

@@ -33,6 +33,7 @@
.method-table, .option-table, .list-table {
td > code {
background-color: inherit;
white-space: pre;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that whitespace is significant, helpers that render inside these code blocks (renderMemberSyntax or similar) need to be more "careful" to avoid:

redundant-whitespace
I manually tweaked the last entry to remove whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 8d644ee

<table class="is-full-width list-table property-table">
<thead>
<tr>
<th>{$ headings[0] or 'Property' $}</th>
{% if hasTypes %}<th>{$ headings[1] or 'Type' $}</th>{% endif %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that headings[1] is not used, it should be "removed" (i.e. remove it from all call-sites that specify it and get the description heading from headings[1] (instead of headings[2])).

@IgorMinar
Copy link
Contributor

IgorMinar commented Sep 25, 2018

  • in the properties table we need to quote the prop value: @Input(foo) -> @Input("foo")
  • presentation of getters and setters should be consistent. I think we should actually drop the "get" and "set" prefix for the props and use display only read-only and write-only if necessary. set foo -> foo write-only

alxhub pushed a commit that referenced this pull request Oct 1, 2018
alxhub pushed a commit that referenced this pull request Oct 1, 2018
`:not(...)` blocks are now rendered as italic, while the
rest of the selector is bold.

PR Close #25768
alxhub pushed a commit that referenced this pull request Oct 1, 2018
If there is additional (non-short) description then add in a
link to the short description to take the reader there.

PR Close #25768
alxhub pushed a commit that referenced this pull request Oct 1, 2018
alxhub pushed a commit that referenced this pull request Oct 1, 2018
Now that `list-table` cells are `pre` formatterd we must be careful
of what whitespace appears in text nodes.

PR Close #25768
alxhub pushed a commit that referenced this pull request Oct 1, 2018
)

(This was added in 405d974 but it is
not clear the reasoning. It looks better to remove it now.)

PR Close #25768
@petebacondarwin petebacondarwin deleted the aio-directve-improvements branch December 12, 2018 08:28
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
If the documentation contains a `@selectors` tag then the content of that
is used to describe the selectors of a directive.

Otherwise the selector string is split and each selector is listed as
a list item in an unordered list.

PR Close angular#25768
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
`:not(...)` blocks are now rendered as italic, while the
rest of the selector is bold.

PR Close angular#25768
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
If there is additional (non-short) description then add in a
link to the short description to take the reader there.

PR Close angular#25768
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
…r#25768)

Now that `list-table` cells are `pre` formatterd we must be careful
of what whitespace appears in text nodes.

PR Close angular#25768
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
…ular#25768)

(This was added in 405d974 but it is
not clear the reasoning. It looks better to remove it now.)

PR Close angular#25768
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours target: patch This PR is targeted for the next patch release
Projects
Development

Successfully merging this pull request may close these issues.

docs-infra: Directive API template improvements aio + docs/api: directive docs ideas
6 participants