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

Do not center table headers by default #144

Merged
merged 4 commits into from Jan 8, 2024

Conversation

dviererbe
Copy link
Contributor

@dviererbe dviererbe commented Nov 15, 2023

The furo base theme centers table headers. This looks weird in my opinion for tables with only a few columns. See for example:

image

The source, so that you can play around with the style yourself:

+-------------------+--------------------+
| lorem             |  ipsum             |
+===================+====================+
| dolor             | sit                |
+-------------------+--------------------+
| amet              | consectetur        |
+-------------------+--------------------+
| adipiscing        | eli                |
+-------------------+--------------------+
| sed               | do                 |
+-------------------+--------------------+

Centering table headers with this change is still possible, for example:

.. role:: center
   :class: align-center

+-------------------+--------------------+
| :center:`lorem`   | :center:`ipsum`    |
+===================+====================+
| dolor             | sit                |
+-------------------+--------------------+
| amet              | consectetur        |
+-------------------+--------------------+
| adipiscing        | eli                |
+-------------------+--------------------+
| sed               | do                 |
+-------------------+--------------------+

This uses the style of table.align-center defined in custom.css:

/* Allow to centre text horizontally in table data cells */
table.align-center {
text-align: center !important;
}

@pmatulis
Copy link
Contributor

I agree with having left-aligned table headers by default. Waiting a bit in case other TAs feel differently.

Copy link
Contributor

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

Left-aligned by default would be my preference as well, the header should follow the column contents unless in specific cases.

Off the top of my head though, I don't think we have documented anywhere about these table options for the benefit of people who might not be as familiar with rst directives. I'll drop an approve on this because it's a good change, but it's worth also adding a note to the rst cheat sheet to show how to get centred header text.

@pmatulis
Copy link
Contributor

Good point @s-makin , and I feel that we should make the cheat-sheet addition in this PR.

Copy link
Collaborator

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I agree that it needs an update to the cheat sheets (both RST and MyST).
We don't have this in the style guide at the moment - do we think it's needed in there?

@dviererbe
Copy link
Contributor Author

Can this be merged?

When the full table is centred, we don't want the headings left-aligned.

Signed-off-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Add examples for centering only some table cells to the cheat
sheet. This also requires a custom role.

Signed-off-by: Ruth Fuchss <ruth.fuchss@canonical.com>
@ru-fu ru-fu self-requested a review January 2, 2024 14:56
@ru-fu
Copy link
Collaborator

ru-fu commented Jan 2, 2024

I added two commits - one to fix a problem that it now wasn't possible anymore to centre a full table, and one to add examples to the cheat sheet.
Are you okay with these additions?

Copy link
Contributor Author

@dviererbe dviererbe left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

Praise: Thanks for adding the center role to the rst_prolog and documenting it! I like that it is defined in custom_conf.py so that downstreams can remove or rename it if wanted/needed.

.sphinx/_static/custom.css Outdated Show resolved Hide resolved
@pmatulis pmatulis merged commit 9bb5387 into canonical:main Jan 8, 2024
2 checks passed
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