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

Docs sphinx 4 upgrade #2666

Merged
merged 11 commits into from Oct 5, 2021
Merged

Docs sphinx 4 upgrade #2666

merged 11 commits into from Oct 5, 2021

Conversation

Grimmys
Copy link
Contributor

@Grimmys Grimmys commented Aug 8, 2021

According to #2598, this PR modify the CSS rules for the docs to be compatible with Sphinx last version (4.1.2)

  • The issue related to the rendering of an image next to a codeblock has been fixed.

  • The rendering of a codeblock has been improved to be similar to what it is actually online.

  • I tried to pay attention to responsive design, Sphinx upgrade let the global appearance much more correct on mobile devices: all the elements are bigger to be more visible, and I added rules to prevent elements from going out of the main container when they are too big.

@Starbuck5
Copy link
Contributor

I'm happy to see a PR for this!

I put docs generated with this and Sphinx 4 next to docs on the main branch generated with Sphinx 3. I didn't really look at your code changes yet, just at the results.

I think this breaks the top level function descriptions when not on mobile.
Capture
The descriptions shouldn't be so bunched, they should be able to expand.

I also noticed some code block problems:
Capture
The line numbers look awfully strange now. (Intro to pygame tutorial)

Capture
What happened to the Sphinx 3 scroll bar? Now it shunts stuff onto the next line, which looks quite bad in some places. (Draw doc)

@Grimmys
Copy link
Contributor Author

Grimmys commented Aug 9, 2021

Ok, thanks for your review.

About the first and the last points, it's my fault, I fixed some responsive issues but ended to the generation of the desktop issues you are showing, I know how to fix them.
Only one question: are you sure we need the scroll bar on mobile devices too? Imo horizontal scroll bar on mobile devices is a bad pattern, like I think it's not easy to use a scroll bar on a small screen... But maybe I'm wrong and it's still better than breaking a line.

About the second point, I'm sorry I can't reproduce the issue, the rendering is like this on my side:

image

Sphinx version: 4.1.2 (last)
Pygments version: 2.9.0 (last)
docutils: 0.17.1 (last)

I think it's mostly Pygments' work to determine syntax highlighting for codeblocks, so maybe you have a different version of it.
If not, please if you can, try to inspect the element on your browser and give me the origin of the rule setting this background.

@Starbuck5
Copy link
Contributor

Alright, nice work, the scroll bar is back, and the top isn't squished, while still supporting phones better, I think.

Your pygments prediction is spot on, that's where the rule is coming from for me:
Capture2
I have pygments version 2.7.3

I also noticed another weird thing-
Capture
This spacing looks quite strange. It is possibly something I missed in the Sphinx 3 transition, but if you want to fix it, epic.

@Grimmys
Copy link
Contributor Author

Grimmys commented Aug 30, 2021

Okay so no problem about pygments, the last stable version of pygments will be used to generate the docs, right?

I also noticed another weird thing-
Capture
This spacing looks quite strange. It is possibly something I missed in the Sphinx 3 transition, but if you want to fix it, epic.

Well... There are a lot of weird spaces in this capture.
About the vertical spacing between "Raises" and "Note", there is an empty line-block that has been defined in the RST file (scrap.rst)... I can remove it, but maybe this "empty line-block" is also defined in other pages (maybe it was a tweak to generate artificial spacing between blocks).

About the horizontal spacing at the end of "Raises:" and "Note:", there are coming from different rules but yeah I think I can do something to make it smoother.

…page and removed some horizontal margin between block title and content
@Starbuck5
Copy link
Contributor

Starbuck5 commented Sep 18, 2021

Alright, I've taken another look at this

To get this working properly, I needed:
Sphinx version: 4.2.0 (latest)
docutils version: 0.16 (0.17 led to no highlighting on the titles of non class modules, like pygame.mouse was worse, but pygame.Surface was fine)
Pygments version: 2.9.0 (latest)

So, with the docutils 0.16 caveat, I think I'm ready to approve this. This limit would need to be documented though. You could continue in another PR? -
"Docutils 0.17 is the new Sphinx 4" ?

171
160

@Grimmys
Copy link
Contributor Author

Grimmys commented Sep 18, 2021

Great.

But I'm wondering ; maybe I could get around the docutils issue in this PR?

This should be quick (one css rule probably), it is somewhat related to the Sphinx 4 upgrade and I think it would be "too much" to add a notice about this issue in the documentation only to remove it in a few months.

@Starbuck5
Copy link
Contributor

That would be wonderful.

The title are under a section tag instead of a div with a "section" class starting from docutils v0.17
Copy link
Contributor

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for working on this @Grimmys! 🎉

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

🎉 thanks. Very nice to be up to date with sphinx again.

@Starbuck5 Starbuck5 merged commit 3d0d25c into pygame:main Oct 5, 2021
@Starbuck5 Starbuck5 added this to the 2.0.2 milestone Oct 5, 2021
@Starbuck5 Starbuck5 added the docs label Oct 5, 2021
@Grimmys Grimmys deleted the docs-sphinx-4-upgrade branch December 12, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants