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

Responsive parameter/... tables #335

Merged
merged 20 commits into from
Dec 7, 2021

Conversation

felixfontein
Copy link
Collaborator

This PR's aim is to convert the HTML blobs to proper RST tables. Well, with still some HTML interspersed so that a) they look good and b) we can do some responsiveness.

The first step is to reduce all tables to two columns, and to convert them to RST. All tables (plugins/modules and roles parameters and return values) are two columns, but only the plugins/modules tables are fully RST currently. The rest will follow.

There's also no CSS collapse from table to 'flat view' (for narrow width devices) yet.

Some pages to compare:

@felixfontein felixfontein changed the title [WIP] Responsive parameter/... tables Responsive parameter/... tables Nov 13, 2021
@felixfontein
Copy link
Collaborator Author

This now covers all tables and is ready for review / public judgement :)

@felixfontein
Copy link
Collaborator Author

(This PR includes #336, will rebase once that - or some equivalent - is merged.)

@briantist
Copy link
Contributor

I have mixed feelings.

  • I love the new formatting of the INI entries; so much easier to see.
  • I would really like to see env vars and ansible vars also rendered as code/fixed-width (like C()).
  • I feel the old layout makes better use of of wider screens, and I like the visual separation of the columns better.
  • The responsiveness is nice, though I think it would be good to retain some of the borders on the small-screen views? Note: it would be very rare for me to browse Ansible documentation on a mobile device or other small screen so I'm not the target audience.
  • While aliases were always kind of difficult to see, and their formatting hasn't changed, their new placement with all the stuff in one cell makes them harder still imo. Before, they were at the bottom of a cell, so there was at least that. Now, they are in the middle of a blob of content. I'm not sure if this was already considered in the past but why not put aliases with the option name itself?
  • I like the addition of (default) to the previously unlabeled arrow within choices listings.
  • Similar to aliases, (non-choices) default values are harder to see and pick out in the new layout. They are rendered smaller, and now lack the visual factor of a dedicated cell (I do agree the cell was mostly a waste of space though). I don't have a good suggestion on how to fix this one...

Question: does using RST tables allow R() to work in option descriptions?

@felixfontein
Copy link
Collaborator Author

* I would really like to see env vars and ansible vars also rendered as code/fixed-width (like `C()`).

That's easy to change.

* The responsiveness is nice, though I think it would be good to retain some of the borders on the small-screen views? Note: it would be very rare for me to browse Ansible documentation on a mobile device or other small screen so I'm not the target audience.

Which borders do you mean?

* While aliases were always kind of difficult to see, and their formatting hasn't changed, their new placement with all the stuff in one cell makes them harder still imo. Before, they were at the bottom of a cell, so there was at least that. Now, they are in the middle of a blob of content. I'm not sure if this was already considered in the past but why not put aliases with the option name itself?

I don't know whether that was considered or not, but I also think it's better to move them to the left column.

Question: does using RST tables allow R() to work in option descriptions?

Yes. (Since everything is RST now.)

@briantist
Copy link
Contributor

  • The responsiveness is nice, though I think it would be good to retain some of the borders on the small-screen views? Note: it would be very rare for me to browse Ansible documentation on a mobile device or other small screen so I'm not the target audience.

Which borders do you mean?

Has borders:
image

When made smaller, borders disappear:
image

@samccann
Copy link
Contributor

So I also like the idea of using the left column a tiny bit more. Definitely adding aliases there seems to make sense to me. I wonder if the non-choices default value would be better in that left column as well? We don't want to overload it, but one or two important additions might be enough?

And love the responsiveness now!

@felixfontein
Copy link
Collaborator Author

@briantist yes I know that, but where do you want borders?

@samccann
Copy link
Contributor

I'm not sure there is a place for borders once they 'disappear'. At that point, it's no longer a table, it is a list. The top of the list is the module name/short description in a gray box, then all parameters are listed below that. Unless we're talking about just adding a box around the whole thing?

@samccann
Copy link
Contributor

fwiw I've share this PR with a few teams to garner feedback since it's a significant change (shrinking out some columns). Should we also post it on the #ansible user chat channels and maybe reddit? or one of the email lists for ansible?

@briantist
Copy link
Contributor

I'm not sure there is a place for borders once they 'disappear'. At that point, it's no longer a table, it is a list. The top of the list is the module name/short description in a gray box, then all parameters are listed below that. Unless we're talking about just adding a box around the whole thing?

I see; well truthfully I'm not sure where to put the borders (or even if that's the right thing to do). I suck massively at design, so I'm more just reporting that I "feel" the lack of the borders; that sense of separation and distinctiveness and the visual separation is gone. I don't know how to best address that.

But I'll also stress again, I personally am very unlikely to ever see that collapsed design naturally in my own use of the docs, so I'm not the target audience and my opinion on it should be taken with a grain of salt... hopefully we'll get more feedback from folks who are more accustomed to reading on small screens/layouts!


Also just a massive "thanks" for thinking about this stuff and putting the work in; I don't want to sound like I'm being too negative :)

@samccann
Copy link
Contributor

Posted to the user chat channel, ansible-project email list, and on reddit to solicit feedback.

@samccann
Copy link
Contributor

comment from irc/matrix user jaqque - "i like the layout. i appreciate that it no longer has a horizontal scroll for the options table. as I don't write the docs, i have no pony in the RTS race"

@felixfontein
Copy link
Collaborator Author

If nobody complains, I'll merge this by tomorrow morning and create a new release of antsibull so we can see how this looks in the devel docs. (This is basically what we decided at today's DaWGs meeting.)

@cidrblock
Copy link

The only thing I noticed in the auth method example above (which I don't think is related to this change now) is that the font size of "Configuration" seems smaller than the text below it (INI entries). Maybe there is an opportunity to use font sizes and bold to better suggest the hierarchy of information within the box

@felixfontein
Copy link
Collaborator Author

@cidrblock it's partially related, since I tried to use similar styles as before. I'm open to suggestions on how to format this better; I'm not good at designing :)

@samccann
Copy link
Contributor

samccann commented Dec 2, 2021

@felixfontein I wonder if it's the ansible-minimal css theme used on your test site? This is what inspection shows me
image

@felixfontein
Copy link
Collaborator Author

I've added a commit and updated my docsite with it which removes the font-size: smaller, so that everything in the Comments column has the same size. (And I fixed a bug I found that formattings like I(...) in returned: didn't work.)

@felixfontein
Copy link
Collaborator Author

Let's discuss this again at today's DaWGs meeting, and if everyone's happy enough with the current layout, let's merge.

@felixfontein
Copy link
Collaborator Author

As discussed in today's DaWGs meeting, I'll merge this now, and this is going into an antsibull release in a couple of days (together with #341 and maybe some more). Then we can create a PR for ansible/ansible to bump the known good version of antsibull for the devel docs build, publish its result to the test docs page, and gather potential more feedback before integrating this into the devel docs.

@felixfontein felixfontein merged commit acfe41e into ansible-community:main Dec 7, 2021
@felixfontein
Copy link
Collaborator Author

@briantist @samccann @acozine @cidrblock and everyone else who provided feedback, thanks a lot!

@felixfontein felixfontein deleted the responsiveness branch December 7, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants