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

Table fixing #60177

Merged
merged 3 commits into from Aug 13, 2019

Conversation

@abadger
Copy link
Member

commented Aug 7, 2019

SUMMARY

Fix tables which were formerly hardcoded

Tables had hardcoded line breaks to workaround a bug in the
read-the-docs theme. Change those so that they now flow according to
the browser size.

Also switch away from grid tables to the simpler to create and read
simple tables

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

This is built on top of #60171 which should be merged first.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@abadger

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

Here's how the old table renders: https://docs.ansible.com/ansible/devel/network/user_guide/platform_eos.html and the new table: https://toshio.fedorapeople.org/ansible/new-platform-eos.png There are a few minor differences which could be addressed if desired (I tried to lay out the new table to be better than the old one but some things can be made more similar to the old table if desired.) The bigger question is whether the rst representation of the table is better this way or as the old grid table. https://github.com/ansible/ansible/pull/60177/files#diff-8c76f8c42cd85480778c6033ce50e8c3

The new, simple table, should be easier to edit as it doesn't have the plethora of lines to keep lined up. However, the old, grid table, might have an advantage when there are so many multi-line entries? Please let me know which rst format we should use for this and similar tables :-) Either way, I can now tweak the rendered form to take advantage of the new stylesheet elements.

@acozine acozine removed the needs_triage label Aug 7, 2019

@acozine

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Looks good. I tested messing up the whitespace, and Sphinx fails spectacularly, so it's easy to tell when you've done something wrong. And tables will be easier to edit without all the dashes and plus signs and so on. We should add something to the style guide about tables, pointing out the |br| and other ansible-specific formatting.

@abadger abadger force-pushed the abadger:table-fixing branch 5 times, most recently Aug 8, 2019

@abadger abadger changed the title [WIP] Table fixing Table fixing Aug 9, 2019

@abadger

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Okay, this PR is now ready for review. It fixes the hardcoded line lengths in all tables in network/user_guide and changes them all from grid tables to simple tables.

@ansibot ansibot added core_review and removed WIP labels Aug 9, 2019

@acozine acozine referenced this pull request Aug 13, 2019

@abadger abadger force-pushed the abadger:table-fixing branch Aug 13, 2019

abadger added some commits Aug 7, 2019

Change formatting of the network/user_guide tables
* Tables had hardcoded line breaks to workaround a bug in the
  read-the-docs theme.  Change those so that they now flow according to
  the browser size.

* Also switch away from grid tables to the simpler to create and read
  simple tables
Changes to table stylesheet
* valign all content, not just the first column.  This becomes important
  with more than two columns
* Set the font-weight to the first <p> element inside of the first
  column.  This allows us to simplify the first column when everything
  there is the attribute name

@abadger abadger force-pushed the abadger:table-fixing branch to 953ee71 Aug 13, 2019

@ansibot ansibot added the has_issue label Aug 13, 2019

@acozine

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Tested again with the rebased branch. The Settings by Platform section looks odd.

In current devel: https://docs.ansible.com/ansible/devel/network/user_guide/platform_index.html#settings-by-platform

With this branch:
Screen Shot 2019-08-13 at 3 52 51 PM

@abadger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Yeah... that's a feature but I don't know if it's good. the table used to have "fake footnotes". They weren't a link, just the ascii * next to each netwok which was Maintained by the Ansible Network Team and then a line after the table which said * Maintained by the Ansible Network Team.

I changed the "fake footnote" into a real footnote and that yields the format that you noticed. It is meaningful, but now both you and I found that we struggled with what that meaning is. What it's saying is "This is footnote 1. It is referenced in 8 different places. (1,2,3,4,5,6,7,8) are links to each of those places".

Would it be better if we used symbol-footnotes here instead of numbered-footnotes? Then the citation at the bottom would render as †(1,2,3,4,5,6,7,8)?

@abadger abadger force-pushed the abadger:table-fixing branch from 9fc565e to a012e95 Aug 13, 2019

@abadger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Doh! You can only do manually specified numeric footnote, not symbolic footnotes. So that won't work.... I can emulate a footnote with a link.... Perhaps that will look best (we won't have the backlink but the backlink is what we think looks odd, right?)

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/network/user_guide/platform_index.rst:45:0: warning: Too many symbol footnote references: only 1 corresponding footnotes available.

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/network/user_guide/platform_index.rst:45:0: Too many symbol footnote references: only 1 corresponding footnotes available.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/network/user_guide/platform_index.rst:45:0: warning: Too many symbol footnote references: only 1 corresponding footnotes available.

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/network/user_guide/platform_index.rst:45:0: Too many symbol footnote references: only 1 corresponding footnotes available.

click here for bot help

Change platform_index footnote from numbered to dagger symbol
The footnote notation was very odd to read.  Try using a symbol for the
footnote instead of a number to see if that will clarify it.  We can't
manually specify symbolled footnotes, though, so we have to emulate a
footnote with an internal link.  This loses the backref to each place
that the footnote was used but since that was the portion which was hard
to read, perhaps that's for the best.

@abadger abadger force-pushed the abadger:table-fixing branch from a012e95 to a5d4e09 Aug 13, 2019

@abadger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Here's the new look for the footnotes:

footnote

@acozine

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

The best of both worlds! This allows the user to click on the dagger symbol and find out immediately what it means, and it's comprehensible to someone who's just reading the line at the bottom.

@acozine acozine merged commit 3f784ca into ansible:devel Aug 13, 2019

1 check passed

Shippable Run 137141 status is SUCCESS.
Details

@abadger abadger deleted the abadger:table-fixing branch Aug 13, 2019

nkatarmal-crest added a commit to ciscoecosystem/ansible that referenced this pull request Aug 22, 2019

Table fixing (ansible#60177)
* Change formatting of the network/user_guide tables

* Tables had hardcoded line breaks to workaround a bug in the
  read-the-docs theme.  Change those so that they now flow according to
  the browser size.

* Also switch away from grid tables to the simpler to create and read
  simple tables

* Changes to table stylesheet

* valign all content, not just the first column.  This becomes important
  with more than two columns
* Set the font-weight to the first <p> element inside of the first
  column.  This allows us to simplify the first column when everything
  there is the attribute name

* Change platform_index footnote from numbered to dagger symbol

The footnote notation was very odd to read.  Try using a symbol for the
footnote instead of a number to see if that will clarify it.  We can't
manually specify symbolled footnotes, though, so we have to emulate a
footnote with an internal link.  This loses the backref to each place
that the footnote was used but since that was the portion which was hard
to read, perhaps that's for the best.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.