-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add compatibility for sphinx4 #1123
Conversation
Sphinx 4.x moves javascript and css declarations to `script_files` and `css_files`. This patch fixes duplicatd link and script tags when using sphinx4 Sphinx 4.x changes the default setting of html_codeblock_linenos_style to 'inline' and deprecates `html_codeblock_linenos_style`. The css for `.hll` to make the hightling span the full line cause the line number and code to be split on seperate lines. I could not get the highlight to span the full line so it was removed completely. I also made line numbers not selectible by the user. This changes also adds the needed changes to the tox testing enviorment alhough, sphinx4 is not officially released to pypi yet. You will need to test these changes locally using: `pip install git+https://github.com/sphinx-doc/sphinx@master`
This reverts commit db30205.
On a quick QA pass across latest in each 1.x, 2.x, 3.x, and 4.x, I found the following issues strictly on Sphinx 4 output (1.8, 2.4, 3.5 were all similar in comparison):
The following examples are Sphinx 3.5 on the left, 4.0 on the right. 123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted some display QA issues separately, but this looks really close. I was expecting more changes. Seems there might be some issue with the JS injection at least, given Mathjax was not happy.
Co-authored-by: Anthony <aj@ohess.org>
…theme into Blendify/sphinx4
I am unable to figure out why the tests aren't passing for older sphinx versions, I tested locally and cannot reproduce. If anyone has any ideas here let me know. |
Actually, I'm not sure how the new versions were working. I believe the issue was that the test cases were not using Pushed up a fix, but only tested against a few versions. CI can do the rest |
Following up on the issues I reported:
I see the issue now. Sphinx 4 seems to request mathjax JS from jsdelivr. uBlock was blocking this for me, allowing solved the issue
I didn't aim for parity here, as the first and last line number block are not easily selectable, but it's very close. I noticed the selectable line numbers still, but rebuilding assets did resolve the selectable text issue locally.
Looks good! I do see a minor spacing issue though. |
Is it still possible that users in production will come across the same issue? I also made a PR for sphinx to expose the sphinx version tuple see: sphinx-doc/sphinx#9447
This seems to be from a mixture of different font types. I have fixed the issue in my recent commit. In the future, I would like to completely redo the API styles in a much more and cleaner way; both visually and the code. Is there anything else still standing? I have noticed there is sometimes an issue with the logo not working but I think we can fix that with #1171 |
This fixes the different spacing issues that were noted.
Many of our PRs have out of date assets, making it hard to test the latest changes. This will at least throw an error on the PR when build assets aren't updated.
Like users not using shinx_rtd_theme as an extension? Yes seems that's a problem with the current implementation, it does require the theme is used as an extension. Might need to rethink this. Edit: rethought. See e6bd3f0
Font types do look different going back to look at 0.5.2 release docs, but don't seem to be the issue I'm noticing: Theme version 0.5.2 looks like Sphinx 1.8 above though. Sphinx 4 seems to have different class names on the domain elements. Remaining
The spacing on the element looks broken back to Sphinx 1.8 + 0.5.2, so not a pressing thing for this PR. It doesn't look great though, so worth fixing separately, maybe for 1.0, or 1.1 |
I implemented the version string -> version_info tuple conversion in Jinja instead. It's not great, but Jinja doesn't have native filters for tuples 🤷 |
@agjohnson so the only thing currently remaining is to increase the padding on the right side of the class names? I am not sure exactly what you meant by your comment. |
The padding is wrong in Sphinx 1.8 and theme 0.5.2. Sphinx 4 and 0.5.2 has the correct padding. We'll want to come back to that, it's been broken for a while. The last issue I noted are the difference in font styles, note the italicized font style on keywords that are highlighted. |
The difference in font style comes down to in sphinx 1.8 these keywords were given the Perhaps we should report the issue to sphinx to hear back if this is a bug or not. The other thing I am going to do is make a PR for the theme to replace the API demo documentation with examples from https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html to ensure we are writing the API signatures in the expected way. Looking at the page it looks like it expecting |
The Here's an example CSS output from Pygments style, you can see the class names aligning with a descriptive name of the type: I'd probably say we'd want to keep the API examples the same for now, so we can trace back errors through releases of the theme. Right now you can go through our previous release docs and see the old examples of API docs. We did discuss building out a wider test matrix for QA testing the theme, and I could see us having stronger QA test docs there. |
@agjohnson okay hopefully everything is good to go now. |
Agreed, that looks good now. We should probably trace back the spacing issue on these, but I see that issue in 0.5.2 anyways, that's not a Sphinx4 change -- actually Sphinx4 looks correct, so 🤷 Thanks for pushing this forward @Blendify ! If we can close the remaining issues, we should be able to get an RC out this week I think. |
@agjohnson I am okay pushing everything else back to the next RC or 1.1 with the exception of #1171 and maybe #813 |
Sphinx 4.x moves javascript and css declarations to
script_files
andcss_files
. This patch fixes duplicated link and script tags when using sphinx4Sphinx 4.x changes the default setting of
html_codeblock_linenos_style
to 'inline' and deprecateshtml_codeblock_linenos_style
. The css for.hll
to make the highlight span the full line causes the line number and code to be split on separate lines. I could not get the highlight to span the full line so it was removed completely. I also made line numbers not selectable by the user.These changes also add the needed changes to the tox testing enviorment alhough, sphinx4 is not officially released to pypi yet. You will need to test these changes locally using:
pip install git+https://github.com/sphinx-doc/sphinx@master