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

Better readthedocs integration #7

Closed
2bndy5 opened this issue Oct 12, 2021 · 13 comments · Fixed by #10
Closed

Better readthedocs integration #7

2bndy5 opened this issue Oct 12, 2021 · 13 comments · Fixed by #10

Comments

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 12, 2021

I've made some mock up edits to better integrate into readthedocs hosted docs. All details can be observed at the issue I raised upstream. Unfortunately, the author has no intention to support RTD. I have 1 more use case to consider before I offer a PR here: when the user disables the nav side menu.

@jbms
Copy link
Owner

jbms commented Oct 12, 2021

Is it possible to use the existing version selection UI that mkdocs-material supports to show the RTD versions?

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 12, 2021

Are you talking about the mike extension?

I was wondering if it could also be done with the versioneer.py UI (I've never tried that feature in sphinx-material). Ultimately, I went this way because of continuity/branding.

I also didn't want to cause any conflicts for future re-basing.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 12, 2021

With the RTD native menu, it is easier for project maintainers to get to their project's settings/build logs on RTD. Otherwise they'd have to edit the URL in the browser or something.

ps - I noticed the "edit this page" icon doesn't show up in this theme (RTD or not).

@jbms
Copy link
Owner

jbms commented Oct 12, 2021

I think it would be reasonable to add support for the native RTD version menu to this theme, since it seems like it can be done in a relatively non-invasive way. In general adding more modifications means more work when merging in changes from mkdocs-material, but I can imagine that RTD may be fairly popular.

I think it would make sense to place it in the top header bar where the other version selector control goes, rather than in the sidebar.

Also you might want to see how to make it fit visually with the theme --- for example in your sample it doesn't display well when using a light rather than dark background.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 12, 2021

Initially, I thought to put it next to the repo info, but I didn't want to steal real estate from the page's title in the header. However, this might be the better solution since the nav side bar and sub-header tabs can be disabled.

I will look into making it more cohesive with the material pallets. Visually, I went with the styling that RTD uses to inject the menu, but now I'm just using the config variables that RTD injects for HTML templates. It might be a little confusing to see the repo's latest release version (in repo info) next the currently viewed docs version/build (in RTD info).

There is a sphinx extension that is supposed to inject all RTD features, but it doesn't cooperate with the material themes. I often see these errors in the browser's dev console:

# log entry
1 GET https://circuitpython-nrf24l01.readthedocs.io/en/rf24-network/_static/documentation_options.js [HTTP/2 404 Not Found 548ms]
2 Loading failed for the <script> with source “https://www.googletagmanager.com/gtag/js?id=UA-17997319-1”.
3 The resource from “https://circuitpython-nrf24l01.readthedocs.io/en/rf24-network/_static/documentation_options.js” was blocked due to MIME type (“text/html”) mismatch (X-Content-Type-Options: nosniff).
4 Loading failed for the <script> with source “https://circuitpython-nrf24l01.readthedocs.io/en/rf24-network/_static/documentation_options.js”. rf24-network:5522:1
5 Uncaught ReferenceError: $ is not defined readthedocs-doc-embed.js:1:24098

RTD builds have this sphinx extension appended automatically (which is where the embed.js and options.js come from). But, the only thing that works is the data that RTD concatenates to conf.py. I'm saying all this to point out that I did look into adding this support without altering the theme (with no resulting joy).

ps - I think RTD generates their own sitemap based on the docs' build info instead of hardcoded repo info...

@jbms
Copy link
Owner

jbms commented Oct 12, 2021

The documentation_options.js issue is separate, I think, though I suppose the RTD extension potentially needs it. This theme excludes that file from the output directory, but it looks like it still needs to be excluded from the HTML templates. The reason it is excluded is that nothing seemed to use it --- it seemed to be mostly for the normal sphinx search implementation, which is not used by this theme.

As far as $, sphinx also includes jquery by default. As this theme does not require it, it is excluded.

Perhaps some config options could be added to allow jquery and/or documentation_options to be retained.

Arguably if you have the version selector you may not want to show the latest release in the repo. Not sure if there is already an option to hide that.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 12, 2021

Thanks for clearing that up a bit. I'm not as proficient in js as I'd like to be, but I can get by.

@jbms
Copy link
Owner

jbms commented Oct 12, 2021

jquery is excluded in these two places:

'_static/jquery.js',

You could see if removing the exclusion allows RTD to work without any other modifications to this theme.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 13, 2021

Great news! I went in and adjusted the sphinx_immaterial/__init__.py to not exclude the jquery stuff (as advised). I should mention that this is done automatically using a env var READTHEDOCS which is only declared on RTD's build environment. Thus no extra interaction required from the user.

It works natively!

image
image

I think I'll do some css overrides to adjust the font size and positioning of the menu (currently gets in the way of the secondary ToC). I'll try to have it "float" just below the header where the repo info is (before the secondary ToC begins).

I'm still seeing some warnings/errors in the browser's console, but they are mostly related to the search feature that this theme replaces. There is still something about google analytics and RTD's use of "ethical ads", but I'm ok with letting those be.

@jbms Your advice saved me a lot more work. Thank you!

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 13, 2021

Overriding the css is done. I'm ready to submit a PR to resolve this.
image
image

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 13, 2021

side note: I just figured out how to fix building latex docs when a project uses this theme. I'm posting it here because it enhances the compatibility with RTD to build PDF and EPUB formats for a RTD project's additional downloads.
image

Technical fix:

def _builder_inited(app: sphinx.application.Sphinx) -> None:
# For compatibility with mkdocs
app.builder.templates.environment.filters['url'] = lambda url: url

needs to be

def _builder_inited(app: sphinx.application.Sphinx) -> None:
    # For compatibility with mkdocs
    if isinstance(app.builder, sphinx.builders.html.StandaloneHTMLBuilder):
        # Latex builder does not have a `templates` attribute
        app.builder.templates.environment.filters["url"] = lambda url: url

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 15, 2021

So, minifying the css overrides for RTD builds is not as important to me as keeping the src dir faithful with the upstream src folder.

As this issue is mostly resolved in __init__.py, the only change I need to address properly is the addition of

/*
 * css overrides for the readthedocs-sphinx-ext that
 * is automatically appended to projects built/hosted on RTD
 */

.rst-versions {
  font-family: var(--md-text-font-family);

  &.rst-badge {
    top: 50px;
    bottom: inherit !important;
    height: auto;
    font-size: .85rem;
  }
}

These rules are only used when the env var READTHEDOCS is asserted. @jbms Is there a way to accomplish this without having to alter the scss files in the src folder (nor tools/build/index.js)?

My first guess would be to explicitly specify a readthedocs.css in the setup.py's setup() function, and let the sphinx build process sort it out.

@jbms
Copy link
Owner

jbms commented Oct 15, 2021

Since it is so small, it is probably simplest to just always include it, rather than add the complexity of conditionally including it.

Adding an additional scss file to be included is not too big of a deal, already there are some additions to account for other sphinx-specific things. It won't cause a lot of merge difficulties.

@2bndy5 2bndy5 mentioned this issue Oct 15, 2021
@jbms jbms closed this as completed in #10 Oct 17, 2021
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 a pull request may close this issue.

2 participants