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

Fix #1190: Add classifier separation on html4 writer #1192

Merged
merged 7 commits into from Aug 12, 2021

Conversation

nienn
Copy link
Contributor

@nienn nienn commented Aug 10, 2021

This fixes #1190. The missing separator exists only as a CSS pseudo element. The selector that creates it is very specific and explicitly excludes the html4 output:

html.writer-html5 .rst-content dl dt span.classifier:before

Solution

Make the selector global:

span.classifier:before

Details

I made the selector global, instead of adding the specific selector container structure required for the html4 case, for two reasons:

  • Did not find span.classifier being used anywhere else, in a different situation that would do badly with this styling.
    Might I be wrong here?
  • I'm not sure about the html structure for the html4 writer.
    Ex1: html4 writer uses tables instead of dls (I did not test this thought)
    Ex2: v 0.5.2 does not add the class .writer-html5, even if it's using dls

@agjohnson agjohnson added this to the 1.0 milestone Aug 10, 2021
Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

In trying to explain some more test cases, I discovered the underlying issue: Sphinx 2 changed the structure of this classifier to match what is output by the HTML5 writer (this isn't specifically an HTML5 feature).

The Sphinx 1.x HTML4 output for the classifier actually had a hardcoded separator:

<dt>Term <span class="classifier-delimiter">:</span> <span class="classifier">classifier</span></dt>

Thinking about this more, perhaps the easiest solution is to always hide dl dt span.class-delimiter, just to keep our selectors more reasonable.

The fix in this PR currently will add an extra separator to Sphinx 1.x HTML4 output -- ie I'd expect Term : : classifier in the output

src/sass/_theme_rst.sass Outdated Show resolved Hide resolved
@nienn
Copy link
Contributor Author

nienn commented Aug 12, 2021

Added the dd dl to make the selector more specific. The .rst-content was also already present.
So now the proposed changes on that selector are just

from:

html.writer-html5 .rst-content dl dt span.classifier:before

to:

.rst-content dl dt span.classifier:before

Also added the rules to hide the hardcoded separator from Sphinx 1.x and prevent duplication of that element:

.rst-content dl dt span.classifier-delimiter {
	display: none !important;
}

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Looks great!

@agjohnson agjohnson merged commit 7500f33 into master Aug 12, 2021
@agjohnson agjohnson deleted the nienn/fix-list-classifier-css branch August 12, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definition list classifier separation missing
3 participants