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

Inherent sphinx base css #747

Closed
wants to merge 1 commit into from
Closed

Inherent sphinx base css #747

wants to merge 1 commit into from

Conversation

Blendify
Copy link
Member

This adds https://github.com/sphinx-doc/sphinx/blob/master/sphinx/themes/basic/static/basic.css_t to our css stack. There might be some side effects so we will have to look at this carefully before merging.

Fix #746

@polyzen
Copy link

polyzen commented Apr 11, 2019

Resolves my issue, and everything looks good in my simple wiki.

@polyzen
Copy link

polyzen commented Jul 18, 2019

Seems like this needs to be assigned so it can be reviewed and merged.

@polyzen
Copy link

polyzen commented Jul 18, 2019

With this, the paragraph icon is inserted just before the anchor icon. Taking a screenshot of hovered elements is too much of a pain, but hopefully this gets the point across:

Important &amp; urgent<a class="headerlink" href="#important-urgent" title="Permalink to this headline">¶</a>

@lesteve
Copy link

lesteve commented Sep 12, 2019

Is there something that could be done to help this PR merged? I completely understand the PR as it stands (i.e. including sphinx basic.css) is a far-reaching change that could have hard-to-detect side-effects.

Maybe a less intrusive change would be to add this to your CSS (diff from sphinx-doc/sphinx#5976) instead?

.classifier:before {
    font-style: normal;
    margin: 0.5em;
    content: ":";
}

As I mentioned in #766 (comment), this would fix the most problematic issue amongst all the Sphinx 2 related issues in sphinx_rtd_theme.

Impact of this issue

Python projects that use sphinx_rtd_theme are affected by this in their auto-generated API doc. This missing delimiter can be very confusing: for example in #766 it looks like the parameter is called eta_efloat instead of eta_e (which is of type float):
image

Here are a few projects I could find that were affected by this (I would guess there are a significantly more):

In all cases above, we have experienced sphinx users (most have used sphinx for years in their projects for their documentation) that have to spend some significant effort to understand where the problem comes from, numpydoc vs sphinx vs sphinx_rtd_theme, and figure out which fix is the least bad of them.

Side-comment: some projects are pinning sphinx < 2 to avoid the problem, except that this is not possible on ReadTheDocs, at least last time I checked, see readthedocs/readthedocs.org#3829 for more details.

@Blendify
Copy link
Member Author

Requesting review, hope this helps, we really need to get sphinx 2.0 support figured out here.

@agjohnson
Copy link
Collaborator

Why extend the base CSS? Wouldn't it be better to fix our styles so that display is correct with the HTML5 writer?

@Blendify
Copy link
Member Author

Blendify commented Sep 26, 2019 via email

@agjohnson
Copy link
Collaborator

I'd rather treat these display issues as bugs with our CSS. Using the base CSS as a mask for these issues would also potentially open up the theme to CSS display issues caused by upstream CSS and CSS changes. Having two layers of CSS rules would also make work on our theme harder, as we don't have independent CSS rules applied anymore, we have to be concerned about two layers of CSS and how selector load order and specificity affect display. There are a few cases for keeping our CSS independent I think, so I'm more enthusiastic about resolving the html5 writer issues with our own CSS fixes.

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Sep 26, 2019
@jessetan
Copy link
Contributor

jessetan commented Oct 3, 2019

I'm +0 on this. While I do like the idea of inheriting Sphinx CSS so that we can benefit from their fixes without manually updating the theme, it does open up the risks described by @agjohnson where we need to work in the other direction: fixing things that broke when we inherited Sphinx' CSS.

Right now the pain of adding Sphinx 2 is limited to a few cases that we can handle manually, although I'll be the first to admit that it is a hassle. If we have many such issues in future, for instance because Sphinx CSS changes more rapidly, inheriting will be the best way.

@agjohnson
Copy link
Collaborator

That might be a good compromise. If we continue to hit issues with the base Sphinx after we address some of the Sphinx 2/HTML5 inconsistencies, we should probably consider extending the Sphinx base css at that point.

It is a hassle, but I do agree that it doesn't seem like we have a wild number of inconsistencies to address. With vacations last month, I just didn't have time to start fixes for the issues we identified. Working on the theme is on my roadmap this month however, so I'll be poking these issues or helping other with them.

@agjohnson
Copy link
Collaborator

I made quick progress on CSS that supports Sphinx 1.x and 2.x. I'm going to close this PR as I think it's adding more variability into our styles compared to explicitly fixing the issues.

@agjohnson agjohnson closed this Oct 19, 2019
@lesteve
Copy link

lesteve commented Oct 19, 2019

I made quick progress on CSS that supports Sphinx 1.x and 2.x.

Great to hear! Can you confirm the work you are referring to is in #838 ?

@stsewd stsewd deleted the basic_css branch December 3, 2020 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classifier delimiter is not shown with Sphinx >=2.0.0
5 participants