Sphinx v4 support: exclude pygments css from layout

[vsalvino]

Noticed a bug caused by my recent change of allowing the user's custom css... it was also pulling in pygments css file (sphinx>=4) which was causing style weirdness.

Blocking that file since our theme.css manually includes pygments styles instead.

Tested the build output on sphinx versions 1, 2, 3, 4 to ensure it looks good on all.

[Scotchester]

Not sure I'm following what's going on here. You added the ability to loop through css_files if present, but it's not set in the included docs/conf.py, so I'm not sure how it would then be bringing in a pygments.css file within that {%- for css in css_files %} loop.

Also not sure what the change is in Sphinx 4 that necessitates that new {%- if sphinx_version_info[0] < 4 %} conditional. I'm sure I'm missing some undertstanding of Sphinx magic that's happening silently under the hood, but all it looks like to me is that users running Sphinx 1, 2, or 3 would not get the theme's default styling?

Sorry, I may not be the most qualified person to review these PRs 😆

[vsalvino]

Yes, this is a bit of a confusing change due to how sphinx changed its behavior in version 4. To your point, the v4 version check is not necessary to exclude pygments, but I just happened to discover that in v4 the theme file was being included twice.

Current code outputted by this theme:

<!-- This is manually hard-coded in layout.html -->
<link rel="stylesheet" type="text/css" href="../_static/dist/theme.css" />
   <link rel="stylesheet" type="text/css" href="../_static/dist/fontawesome.css" />
<!-- These are coming from css_files -->
   <link rel="stylesheet" type="text/css" href="../_static/pygments.css" />
   <link rel="stylesheet" type="text/css" href="../_static/dist/theme.css" />
   <link rel="stylesheet" type="text/css" href="https://assets.readthedocs.org/static/css/badge_only.css" />

Prior to v4:

• The theme must manually add its own CSS files in the layout.html template. This is all-encompassing including pygments.
• ONLY user-added CSS files get put in css_files

After v4:

• The theme should NOT specify its primary CSS file, or any included sphinx files (pygments) in the layout.html.
• Those files, plus any user-added CSS files, get automatically put in css_files.

See diff in layout.html between v3 and v4: sphinx-doc/sphinx@5b392e3#diff-a5066e933cbf65adc46e0d1ab9a0b44e0a53ca64cc95dca7e6aa902aed6bd468

I am using RTD theme as a reference point, since it is actively maintained: https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/layout.html

[vsalvino]

@tbrlpld could I get a quick review/approval to merge this in?

[tbrlpld]

@vsalvino Trying to wrap my head around this at the moment. Could we fix the indentation so that template tags also cause a level of indentation?

E.g.

{% for item in the_list %}
    <li>{ item }</li>
{% endfor %}

instead of

{% for item in the_list %}
<li>{ item }</li>
{% endfor %}

[Stormheg]

I think I get what this does but I'm wondering if there is a better/smarter way to do this. This feels like swimming against the current so to speak 😉

Possibly we could extract our pygments styling and include it through the pygments_style option? That way we...

• ...probably wouldn't need a 'hack' to exclude Sphinx's default pygments style.
• ...would allow users of our theme to provide their own pygments styles without ours conflicting.

What do you think @vsalvino? Would the above be a viable option? Happy to hear your thoughts since you appear to have more experience with Sphinx than me.

[allcaps]

@lb- Can this be closed now? Ref 33bf537

[lb-]

Yeah I think so. I didn't even see this sorry.
Now that we are just on the 'latest' Sphinx we don't need the versioning logic.

I'll close and @vsalvino please review the commit reference above and let us know if we need further work done.

[vsalvino]

The templating logic in this PR is still needed.

If you run the package as-is without this change, using Sphinx version 4, you will see broken links (404s) in the stylesheet links in the <head>

If you run the package as-is without this change, using Sphinx older than 4, you will see the wrong styles in the source code syntax highlighting. As sphinx will still be pulling in the default pygments style.

It might be a good idea to render the docs from the main branch, on both versions of sphinx, and confirm those two issues. If they are not issues in the main branch then this PR can be ignored.

[lb-]

@vsalvino thanks for responding - and it looks like your changes have been picked up in #154

Can you please have a read of that PR

[tbrlpld]

Are older versions of Sphinx still being maintained? Can we maybe just drop support for them?

[vsalvino]

Read the docs defaults to Sphinx 1.6. So it is likely that we will want to continue support. Alternatively we can specify only sphinx v4 or higher. There are fields for this both in setup.py and also in the sphinx theme config.

[lb-]

We could do the following.

• a new issue on this repo to explain how to change the config with readthedocs / change our supported version
• a new issue on Wagtail (and other repos) that they need to update their readthedocs config - probably best these are actioned first.