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

Margins are broken in bullet lists on 0.5.0rc1 #916

Closed
Jackenmen opened this issue May 7, 2020 · 6 comments · Fixed by #939
Closed

Margins are broken in bullet lists on 0.5.0rc1 #916

Jackenmen opened this issue May 7, 2020 · 6 comments · Fixed by #939
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@Jackenmen
Copy link

Jackenmen commented May 7, 2020

Problem

Margins in bullet lists are extremely large on 0.5.0rc1
Happens on both Sphinx 2.4.4 and Sphinx 3.0.3

Reproducible Project

RTD 0.4.3, Sphinx 2.4.4 (how it should look):
https://docs.discord.red/en/latest/guide_publish_cogs.html

RTD 0.5.0rc1, Sphinx 2.4.4 & 3.0.3 (broken margins):
https://jack1142-red-prs.readthedocs.io/en/v3-bump_deps/guide_publish_cogs.html

Error Logs/Results

RTD 0.5.0rc1, Sphinx 2.4.4 & 3.0.3 (broken margins):
image

Expected Results

RTD 0.4.3, Sphinx 2.4.4 (how it should look):
image

Environment Info

  • Python Version: 3.8.0
  • Sphinx Version: 2.4.4 & 3.0.3
  • RTD Theme Version: 0.5.0rc1
@agjohnson
Copy link
Collaborator

Thanks for the report! You actually look to be using the html4 writer still, does the output improve with the html5 writer?

The bug here looks to be a nested <p> element and a selector adding margin to :last-child. This in theory shouldn't have changed, but likely there wasn't a test case with a <ul><li><p>... structure.

@agjohnson agjohnson added Accepted Accepted issue on our roadmap Bug A bug labels May 7, 2020
@agjohnson agjohnson added this to the 0.5 milestone May 7, 2020
@Jackenmen
Copy link
Author

@agjohnson That particular thing looks better on html5 writer, though it's not perfect - margins for the "tip box" are a little off:
image

Link: https://jack1142-red-prs.readthedocs.io/en/v3-bump_deps_html5_writer/guide_publish_cogs.html

For now I just wanted to see if I can upgrade Sphinx without using new writer, I will move to it eventually, but there are still some problems with it like:

I can make issues for those, though I didn't plan to look into html5 writer just yet 😄

@agjohnson
Copy link
Collaborator

Great, that's all very helpful. Seems we need to get some more test cases into the demo docs.

That last one is a good example -- a definition list with multiple words (?) i believe gets a different class (dl.simple) than other definition lists. There is already a fix merged for that one at least, #914.

@Jackenmen
Copy link
Author

I should probably make a separate issue for the second (or add a comment on some existing issue if you've got any pointers), the third one is already fixed in master as you said and first one might be related to this issue.

@agjohnson
Copy link
Collaborator

I've opened #939 to address the spacing issues.

I noticed in building a test case that your examples are rendering as blockquotes inside list elements, which will affect display a bit more than simply nesting. This is due to the spacing being different between the reST bullet list and the paragraph/admonition blocks that follow the initial list item text.

I made our test case use the list spacing, not blockquote spacing, but things look more consistent now. I didn't test how this might affect the blockquote usage nested in a list item.

I'm going to treat #939 as the fix to this issue, but perhaps if there is something to change after a 0.5.0rc2 we can decide if it needs to be a theme fix or not.

@Jackenmen
Copy link
Author

@agjohnson I'm not entirely sure, is it intended that the margins here are now larger?

0.5.0rc2:
image

0.4.3:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants