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

webdocs: Differentiate between type and required #42888

Closed
wants to merge 2 commits into from

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Jul 17, 2018

SUMMARY

Now that parameters have their (non-string) type indicated in the module
docs, it becomes harder to tell which parameter is required, and what is
the type. (Mostly because both are red and positioned similarly).

screenshot from 2018-07-17 13-54-35

Furthermore, if a non-string parameter is required, both are now
indicated subsequently.

screenshot from 2018-07-17 13-56-24

This change makes required parameters stand out by adding asterisks to
the parameter name (in red) with an explanation at the bottom.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

Module docs

ANSIBLE VERSION

v2.7

Now that parameters have their (non-string) type indicated in the module
docs, it becomes harder to tell which parameter is required, and what is
the type. (Mostly because both are red and positioned similarly).

Furthermore, if a non-string parameter is required, both are now
indicated subsequently.

This change makes required parameters stand out by adding asterisks to
the parameter name (in red) with an explanation at the bottom.
@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 docs This issue/PR relates to or includes documentation. needs_triage Needs a first human triage before being processed. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 17, 2018
@dagwieers
Copy link
Contributor Author

This change would result in:
screenshot from 2018-07-17 14-53-09

And
screenshot from 2018-07-17 14-53-26

@dagwieers
Copy link
Contributor Author

cc @felixfontein

@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label Jul 17, 2018
@gundalow gundalow added this to To do in OLD Ansible Documentation via automation Jul 17, 2018
@dagwieers dagwieers requested a review from gundalow July 17, 2018 14:42
@jborean93
Copy link
Contributor

IMO I like required being in the cell and not using the asterisk version, makes it a lot easier to see.

@felixfontein
Copy link
Contributor

I agree that the current version looks ugly (especially when both required and the type are shown for an option), but I also agree with @jborean93 that having required written out in the cell makes it easier to see (especially for newbies who aren't used to the asterisk yet).

How about changing the styles instead? (I think we should also start using CSS classes instead of directly formatting everything inline :) )

An idea: use different colors for types and required. I'd keep required as red, but change the type to something else. Maybe blue (like the default values in the adjacent cell)? Also, if both required and type appear, the empty line between them should go away.

@dagwieers
Copy link
Contributor Author

dagwieers commented Jul 18, 2018

You have to consider that the first cell holds 4 pieces of information currently:

  • parameter name -- black, bold
  • required (optional) -- red
  • type (optional now, but becomes mandatory at some point) -- red
  • version_added (infrequent) -- darkgreen

I only noticed myself recently that someone added parameter type (with the same color) to the first cell, just like was already practice for result values.

So the usability problems are:

  • None of the 3 colored values are always available (although over time we expect the parameter type to be set everywhere)
  • All 3 colored values could be available for a specific parameter
  • required is more important, and should stand out (preferably first)

So the easiest to make a parameter stand out is using asterisks (this is a common convention, using asterisks means a required field). This doesn't break the visual convention because it's positioned elsewhere. That's why it was done like this, to keep things consistent.

@dagwieers
Copy link
Contributor Author

@felixfontein We should be using CSS, however we were told earlier nobody was allowed to touch the CSS styles, so we started doing it like this (eventually it being turned into CSS by @dharmabumstead).

@dagwieers
Copy link
Contributor Author

dagwieers commented Jul 18, 2018

Someone proposed adding another column, but it used to be like that and more columns is not the best use of screen-estate.

See:

@bcoca
Copy link
Member

bcoca commented Jul 18, 2018

making the name of required fields bold or red (both?) should work, as long as on top of the table we have 'X means the field is required' .. which is a pretty common pattern in docs.

@felixfontein
Copy link
Contributor

@dagwieers I think I added the type name, following a suggestion by you (see #41999 and #41905).

I fully agree that having another column is not good either :)

@bcoca I wouldn't make it red, because we already use red for parameter types. But bold seems like a good idea as well (essentially what @dagwieers proposed, except that it is bold instead of having an asterisk).

Maybe we should make it both bold and add an asterisk?

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 27, 2018
@acozine acozine moved this from To do to In progress in OLD Ansible Documentation Jul 31, 2018
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 31, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 8, 2018
@dagwieers
Copy link
Contributor Author

Coming back to this. When a parameter is required, it does not have a default.
Which means that we could use the second column also to indicate that the parameter is required.
Only when choices exist without a default, there would be 2 things in the second column.
But maybe we can harmonize both visually ?

@ansibot ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label Oct 24, 2018
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 12, 2018
@dagwieers
Copy link
Contributor Author

This is superseded by #49966.

@dagwieers dagwieers closed this Dec 18, 2018
@acozine acozine removed this from In progress in OLD Ansible Documentation Jan 28, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants