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.

Sign up for GitHub

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

Jump to bottom

docs-infra: aio CLI boolean options #26272

Closed
wants to merge 4 commits into from
Closed

docs-infra: aio CLI boolean options #26272

wants to merge 4 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 5, 2018 •
edited

These are the changes based on offline discussions from 4 Oct 18.

Notably, we only show the "canonical" syntax for each option:

  • the primary name of the option
  • the list of values (including true/false for boolean)
  • no visual indication of the default value

Then the default value and any aliases are listed at the end of the description.

Additionally, if the value (or list of values) is over 15 character long, then we move that to a new (indented) line, to prevent the description column from being squashed.

Here are some examples:

screen shot 2018-10-05 at 14 06 10

screen shot 2018-10-05 at 14 07 57

screen shot 2018-10-05 at 14 07 52

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours freq2: medium comp: docs-infra target: major This PR is targeted for the next major release labels Oct 5, 2018
@petebacondarwin petebacondarwin added this to the Backlog milestone Oct 5, 2018
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Oct 5, 2018
@mary-poppins
Copy link

You can preview d9a2bf4 at https://pr26272-d9a2bf4.ngbuilds.io/.

@Splaktar
Copy link
Member

Splaktar commented Oct 5, 2018 •
edited

Should we consider 13 or 14 chars being the cutoff? 15 is a bit too long for a phone with width of 425px.

screen shot 2018-10-05 at 2 49 13 pm

Worse on an iPhone:
screen shot 2018-10-05 at 2 51 15 pm

Splaktar
Splaktar reviewed Oct 5, 2018
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Looks much less confusing!

@petebacondarwin
Copy link
Member Author

We are going to fix tables generally to be more responsive for mobile layouts in the not-too-distant future. So that should make that better, generally for this situation.

@jenniferfell
Copy link
Contributor

I really like this. My only comment is that the blue italic looks like a hyperlink. And italic is used to mean something variable in other parts of the doc. Maybe just regular font?

jenniferfell
jenniferfell reviewed Oct 18, 2018
View reviewed changes
Copy link
Contributor

@jenniferfell jenniferfell left a comment

Choose a reason for hiding this comment

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

See comment above.

@mary-poppins
Copy link

You can preview 7c3f92c at https://pr26272-7c3f92c.ngbuilds.io/.

Splaktar
Splaktar approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@petebacondarwin petebacondarwin added this to REVIEW in docs-infra Oct 19, 2018
@petebacondarwin petebacondarwin added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Oct 19, 2018
jenniferfell
jenniferfell approved these changes Oct 19, 2018
View reviewed changes
Copy link
Contributor

@jenniferfell jenniferfell left a comment

Choose a reason for hiding this comment

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

LGTM

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 19, 2018
@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Oct 19, 2018
@alxhub alxhub closed this in 6c530d3 Oct 19, 2018
alxhub pushed a commit that referenced this pull request Oct 19, 2018
@petebacondarwin
build(docs-infra): update CLI option rendering (#26272)
3f89aeb 
PR Close #26272
alxhub pushed a commit that referenced this pull request Oct 19, 2018
@petebacondarwin
build(docs-infra): break long CLI options onto two lines (#26272)
b9bd95b 
PR Close #26272
alxhub pushed a commit that referenced this pull request Oct 19, 2018
@petebacondarwin
build(docs-infra): render CLI arguments consistently (#26272)
dca176e 
In the command syntax, arguments are rendered as
`var`s enclosed in angle brackets. So this is now repeated
in the arguments table too.

PR Close #26272
alxhub pushed a commit that referenced this pull request Oct 19, 2018
@petebacondarwin
build(docs-infra): update CLI option rendering (#26272)
6d240cf 
PR Close #26272
alxhub pushed a commit that referenced this pull request Oct 19, 2018
@petebacondarwin
build(docs-infra): break long CLI options onto two lines (#26272)
658a18c 
PR Close #26272
@petebacondarwin petebacondarwin deleted the aio-cli-boolean-options branch October 19, 2018 20:40
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
@petebacondarwin @sculove
build(docs-infra): render CLI arguments consistently (angular#26272)
a98cfa4 
In the command syntax, arguments are rendered as
`var`s enclosed in angle brackets. So this is now repeated
in the arguments table too.

PR Close angular#26272
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
@petebacondarwin @sculove
build(docs-infra): update CLI option rendering (angular#26272)
4db54bd 
PR Close angular#26272
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
@petebacondarwin @sculove
build(docs-infra): break long CLI options onto two lines (angular#26272)
8589e8c 
PR Close angular#26272
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@petebacondarwin @FrederikSchlemmer
build(docs-infra): render CLI arguments consistently (angular#26272)
ca2f629 
In the command syntax, arguments are rendered as
`var`s enclosed in angle brackets. So this is now repeated
in the arguments table too.

PR Close angular#26272
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@petebacondarwin @FrederikSchlemmer
build(docs-infra): update CLI option rendering (angular#26272)
8393232 
PR Close angular#26272
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@petebacondarwin @FrederikSchlemmer
build(docs-infra): break long CLI options onto two lines (angular#26272)
19b84c1 
PR Close angular#26272
@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.
Reviewers

@Splaktar Splaktar Splaktar approved these changes

@jenniferfell jenniferfell jenniferfell approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours freq2: medium target: patch This PR is targeted for the next patch release
Projects
docs-infra
MERGE
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@petebacondarwin @mary-poppins @Splaktar @jenniferfell @googlebot