Skip to content

build(aio): rendering of optional parameters#22435

Closed
petebacondarwin wants to merge 2 commits intoangular:masterfrom
petebacondarwin:aio-render-parameter-optionality
Closed

build(aio): rendering of optional parameters#22435
petebacondarwin wants to merge 2 commits intoangular:masterfrom
petebacondarwin:aio-render-parameter-optionality

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Feb 24, 2018

Closes #22134

Now we automatically render Optional. at the start of the parameter description if the parameter is not required. We also render the value given to the parameter if it is not provided in the call.

An example is in the Location class... (https://pr22435-d01479e.ngbuilds.io/api/common/Location#path)

screen shot 2018-02-25 at 09 24 44

Note that if there is no description for the parameter then we currently just display the type of the parameter. This is up for negotiation.

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer comp: aio target: major This PR is targeted for the next major release labels Feb 24, 2018
@mary-poppins
Copy link

You can preview a96747a at https://pr22435-a96747a.ngbuilds.io/.

@petebacondarwin petebacondarwin changed the title build(aio): render whether parameters are optional build(aio): rendering of optional parameters Feb 25, 2018
@mary-poppins
Copy link

You can preview d01479e at https://pr22435-d01479e.ngbuilds.io/.

<td class="param-description">
{% if parameter.description | trim %}{$ parameter.description | marked $}
{% marked %}
{% if parameter.isOptional or parameter.defaultValue !== undefined %}Optional. Default is `{$ parameter.defaultValue === undefined and 'undefined' or parameter.defaultValue $}`.{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

I would consider placing this after the description/type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the example given in @jbogarthyde's Google doc "Animations API example".

screen shot 2018-02-25 at 20 55 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can change the ordering around later, as we get more eyes on this - especially when parameters actually start getting descriptions :-P . Meanwhile the question of whether/where to show the type of the param is more problematic IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Normally, I would suggest having the type in a separate column. But given the fact that some types will be too verbose and that the types are already shown in the signature, I think it is fine to just show the description.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 25, 2018
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 25, 2018
@alexeagle alexeagle closed this in 1ea41d4 Feb 26, 2018
alexeagle pushed a commit that referenced this pull request Feb 26, 2018
@petebacondarwin petebacondarwin deleted the aio-render-parameter-optionality branch February 27, 2018 09:37
@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 13, 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 target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aio + docs/api: indicate when a param is optional

4 participants