Skip to content

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

Merged
choldgraf merged 1 commit intoexecutablebooks:masterfrom
feanil:feanil/fix_logo
Jan 6, 2023
Merged

[FIX] Use logo url as is to allow for web urls.#661
choldgraf merged 1 commit intoexecutablebooks:masterfrom
feanil:feanil/fix_logo

Conversation

@feanil
Copy link
Copy Markdown

@feanil feanil commented Dec 21, 2022

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

fix: Use logo url as is.
0da1033 
`logo_url` can sometimes be a fully qualified URL starting in Sphinx 4.
Adding the `_static` prefix breaks logos that are URLs.
@welcome
Copy link
Copy Markdown

welcome bot commented Dec 21, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

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! 🎉

@feanil feanil changed the title fix: Use logo url as is. [FIX] Use logo url as is to allow for web urls. Dec 21, 2022
@choldgraf
Copy link
Copy Markdown
Member

choldgraf commented Jan 5, 2023

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
Copy link
Copy Markdown
Author

feanil commented Jan 5, 2023

@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
pradyunsg reviewed Jan 5, 2023
View reviewed changes
src/sphinx_book_theme/theme/sphinx_book_theme/components/sidebar-logo.html
{% endif %}
{% if logo_url %}
<img src="{{ pathto('_static/' + logo_url, 1) }}" class="logo" alt="logo">
<img src="{{ logo_url|e }}" class="logo" alt="logo">
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg Jan 5, 2023

Choose a reason for hiding this comment

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

Why is this being escaped? Isn't it an already-encoded URL?

Copy link
Copy Markdown
Author

@feanil feanil Jan 5, 2023

Choose a reason for hiding this comment

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

@pradyunsg I don't think that's guaranteed since the logo_url value comes from the user in conf.py and the code I linked to above doesn't seem to be escaping anything.

Copy link
Copy Markdown
Member

@pradyunsg pradyunsg Jan 5, 2023

Choose a reason for hiding this comment

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

You're correct and I have to go fix something in Furo. :)

x-ref: https://github.com/sphinx-doc/sphinx/blob/6e6b9ad45194177811551c77f5f4040213d1c661/sphinx/themes/basic/layout.html#L56

@pradyunsg
Copy link
Copy Markdown
Member

pradyunsg commented Jan 5, 2023

x-ref pradyunsg/furo#253 where I'd discovered this as well. :)

choldgraf
choldgraf approved these changes Jan 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Many thanks for this fix and for linking to the sphinx code where this is now handled! :-)

@choldgraf choldgraf merged commit 0a16df6 into executablebooks:master Jan 6, 2023
@welcome
Copy link
Copy Markdown

welcome bot commented Jan 6, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@feanil feanil deleted the feanil/fix_logo branch January 6, 2023 18:28
@feanil
Copy link
Copy Markdown
Author

feanil commented Jan 6, 2023

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

@choldgraf
Copy link
Copy Markdown
Member

choldgraf commented Jan 7, 2023

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

@melund
Copy link
Copy Markdown

melund commented Jan 18, 2023 •
edited
Loading

@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
Copy link
Copy Markdown
Author

feanil commented Jan 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pradyunsg pradyunsg pradyunsg left review comments

@choldgraf choldgraf choldgraf approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@feanil @choldgraf @pradyunsg @melund