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
Footer: return well formed html #8202
Conversation
@@ -24,6 +24,10 @@ | |||
margin: 0; | |||
} | |||
|
|||
.rtd-current-item { |
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.
noticed we are using rst-
for the other selectors, not sure why, so I went with rtd-
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 think the CSS for the footer is defined here: https://github.com/readthedocs/readthedocs.org/blob/master/readthedocs/core/static/core/css/badge_only.css
Which is built from here: https://github.com/readthedocs/sphinx_rtd_theme/blob/master/src/sass/badge_only.sass
But we should fix that and include this CSS as part of RTD, but we should put the class in the right place.
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.
Done readthedocs/sphinx_rtd_theme#1148, should I just copy that file here? or is it done with a script?
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.
...no idea -- looks like it hasn't changed since 2018 :D
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'm sure there was a script at some point, but just manually copying it is probably fine.
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.
Hmm, I only see this file being included in our docs https://assets.readthedocs.org/static/css/readthedocs-doc-embed.css
In a mkdocs project both files are included (https://assets.readthedocs.org/static/css/badge_only.css)
https://rtd-mkdocs-test-project.readthedocs.io/en/readthedocs-1.0/
readthedocs.org/readthedocs/doc_builder/backends/mkdocs.py
Lines 169 to 172 in f4c0a21
extra_css_list = [ | |
'%scss/badge_only.css' % static_url, | |
'%scss/readthedocs-doc-embed.css' % static_url, | |
] |
So, looks like we need to include that change here after all p:
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.
@stsewd badge_only is included on non-RTD theme docs, eg. https://pip.pypa.io/en/stable/
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.
Created #8247
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.
Footer change looks good, but should put the CSS in the right place.
@@ -24,6 +24,10 @@ | |||
margin: 0; | |||
} | |||
|
|||
.rtd-current-item { |
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 think the CSS for the footer is defined here: https://github.com/readthedocs/readthedocs.org/blob/master/readthedocs/core/static/core/css/badge_only.css
Which is built from here: https://github.com/readthedocs/sphinx_rtd_theme/blob/master/src/sass/badge_only.sass
But we should fix that and include this CSS as part of RTD, but we should put the class in the right place.
We shouldn't use strong tags to surround a dd tag, use css instead. Closes #7903
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 feel like we shouldn't hold up this PR on the small CSS change. Let's open an issue to make this way more obvious, and merge this 👍
We shouldn't use strong tags to surround a dd tag, use css instead.
Closes #7903