executablebooks / sphinx-book-theme

A clean book theme for scientific explanations and documentation with Sphinx
https://sphinx-book-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
431 stars 197 forks source link

[FIX] Use logo url as is to allow for web urls. #661

Closed feanil closed 1 year ago

feanil commented 1 year ago

logo_url can sometimes be a fully qualified URL starting in Sphinx 4. Adding the _static prefix breaks logos that are URLs.

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

choldgraf commented 1 year ago

Thanks for the PR - one question, won't this break local paths because we're removing the pathto? It seems we should instead want to use pathto if the variable doesn't start with http.

feanil commented 1 year ago

@choldgraf the HTML builder handles this in Sphinx 4+

master: https://github.com/sphinx-doc/sphinx/blob/master/sphinx/builders/html/__init__.py#L1254-L1255 v4.0.0: https://github.com/sphinx-doc/sphinx/blob/v4.0.0/sphinx/builders/html/__init__.py#L1191-L1195

pradyunsg commented 1 year ago

x-ref https://github.com/pradyunsg/furo/pull/253 where I'd discovered this as well. :)

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart:

feanil commented 1 year ago

Thanks for the quick reviews @choldgraf and @pradyunsg, when do you guys think this will be released in a new version?

choldgraf commented 1 year ago

There's a release candidate out now and this will make it into the next RC i think

melund commented 1 year ago

@fenil/@choldgraf The master version of sphinx-book-theme doesn't use the sidebar-logo.htm anymore, but instead the navbar-logo.html from the pydata theme.

That version does check that the if the string is an URL, but if not it prepends _static/ which causes me to see it prepended twice.
image

A workaround is to specify the logo using the PyData logo config (light and dark theme). I guess that prevents sphinx from also prepending the _static:

html_theme_options: Dict[str, Any] = {
      ...,
    "logo": {
      "image_light": "logo.svg",
      "image_dark": "logo.svg",
   },
feanil commented 1 year ago

@melund it sounds like maybe this same fix needs to be made in navbar-logo.html as well?