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

sites: Add support for readthedocs.io #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Holzhaus
Copy link
Contributor

@Holzhaus Holzhaus commented Jul 7, 2019

Add some fixes for readthedocs.io.

Screenshots (Gruvbox):

Before
After

@alphapapa
Copy link
Owner

Hi Jan,

Thanks also for this PR. A few notes:

  1. In the screenshot, could you improve the contrast of the Configuration File line in the nav sidebar?
  2. Below that, the Version 2 and Version 1 are the same background color as the rest of the sidebar. Is that how it is in the default RtD theme, or should those sub-headings have a different background color? Not that we have to follow it exactly, just a thought.
  3. The CSS selectors are very specific and verbose. This isn't necessarily bad, but it might be preferable to make them as simple as possible. For example, instead of .rst-content .wy-alert-info.admonition .wy-alert-title, would .wy-alert-title be sufficient?
  4. I also made a line review comment about a possible missing comma.

Thanks.

.rst-content .note .wy-alert-title,
.rst-content .seealso .admonition-title,
.rst-content .seealso .wy-alert-title,
.rst-content .wy-alert-info.admonition .admonition-title
Copy link
Owner

@alphapapa alphapapa Jul 7, 2019

Choose a reason for hiding this comment

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

Is a comma missing at the end of this line?

Or, rather, are any of these commas necessary? I don't think they are, since it's Stylus.

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.

2 participants