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

basic CSS: Take "simple" lists into account, resuscitating the "html_compact_lists" config option #7852

Merged
merged 3 commits into from Jul 4, 2020

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Jun 18, 2020

See #7838, where the CSS classes compact and simple have been mentioned.

I'm not sure if that's the best solution, but it should definitely be a basis for further discussion.

This is how it looks with the docutils demo https://github.com/docutils/docutils/blob/master/docutils/docs/user/rst/demo.txt, using the sphinxdoc theme:

Before:

image

Between (Sphinx 3.1):

image

After:

image


There is still something strange happening in the last case: The list containing "Third level." and "Item 2." doesn't get the simple class, but the one containing it does.
I think those two cases should be reversed, right?
I guess that's a bug?

@mgeier
Copy link
Contributor Author

mgeier commented Jun 18, 2020

For comparison, the rendering of the docutils demo from https://docutils.sourceforge.io/docs/user/rst/demo.html#bullet-lists, which isn't quite consistent, either:

image

@jakobandersen
Copy link
Contributor

  • I don't think the compact class should be used in the CSS due to redundancy. If it is present, the simple class should get set anyway.
  • The docutils page you link to seem to be generated with the HTML 4 writer (no paragraphs in list items).
  • The inconsistency seems to be on purpose in docutils: https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/_html_base.py#l527, only set simple if the current list is simple and a the nearest parent isn't.
    However, this is only happening in the bullet list, not for enumerated lists. I suggest we overwrite this particular function visit_bullet_list and make it unconditionally set simple on each list that is simple.

@mgeier
Copy link
Contributor Author

mgeier commented Jun 19, 2020

Thanks @jakobandersen, I've removed the compact class in 0051992.

The other things you mention sound good, but they should be done in a separate issue/PR, right?

@jakobandersen
Copy link
Contributor

I'm not sure. In a sense it belongs together with this, and probably also the other general CSS rules for lists discussed in #7838. It's not immediately clear to me which rules are really needed in the end, and the exact reasoning behind them all.

@mgeier mgeier changed the title basic CSS: Take "simple" and "compact" lists into account basic CSS: Take "simple" lists into account Jun 20, 2020
@mgeier
Copy link
Contributor Author

mgeier commented Jun 20, 2020

I thought that "compact" lists can contain "non-compact" sub-lists. I was wrong.

I read again the definition at https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_compact_lists:

If true, a list all whose items consist of a single paragraph and/or a sub-list all whose items etc… (recursive definition) will not use the <p> element for any of its items.

This implies, from a CSS point of view, that the simple class also applies to nested lists.
There is no need for nested lists to have the simple class if a parent list has it (but of course it doesn't hurt if they do have the simple class, it's just redundant).

Also I previously wasn't completely aware of the "and/or" part of the definition.

With this new (for me) knowledge, I made some adaptations to the CSS in this PR: 82768d8.

With these changes, no changes to the Python code are necessary and I think it finally looks exactly as intended:

image

Note the correct space after compact lists which are nested within non-compact lists.

Also note the correct space between the separate lists in the last list item.

The space between "1. Arabic numerals." and "a. lower alpha)" might seem out of place given the further sub-lists, but I think it's actually correct. The main list (numbered 1. and 2.) is "non-compact" (because item 2 contains more than "a single paragraph and/or a [single] sub-list") while the nested lists (a., i., A., I.) are "compact".

BTW, the html_compact_lists = False option also works correctly:

image

@mgeier mgeier changed the title basic CSS: Take "simple" lists into account basic CSS: Take "simple" lists into account, resuscitating the "html_compact_lists" config option Jun 20, 2020
@mgeier mgeier changed the base branch from 3.x to 3.1.x June 20, 2020 19:47
@return42
Copy link

@mgeier wrote #7657 (comment)

@return42 Please check out #7852, which should fix the list spacing issues.

I am very sorry, I have my own timeline in another Open-Source project .. thanks for your investigation! For the future you better might pick some tests from wider docs .. e.g. the themes from poco often used .. or use the one from us :)

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Sorry for late. Looks good to me. So merging this now.

@tk0miya tk0miya merged commit 2cc6264 into sphinx-doc:3.1.x Jul 4, 2020
tk0miya added a commit that referenced this pull request Jul 4, 2020
@mgeier mgeier deleted the basic-css-simple-lists branch July 4, 2020 18:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants