Skip to content

logo_url theme configuration option #978

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

logo_url theme configuration option #978

wants to merge 4 commits into from

Conversation

simonw
Copy link

@simonw simonw commented Jul 12, 2020 •
edited by stsewd
Loading

For customizing the page that the logo at the top of the navigation menu links to. Closes #977

JamesRandom
JamesRandom approved these changes Jul 15, 2020
View reviewed changes
Copy link

@JamesRandom JamesRandom left a comment

Choose a reason for hiding this comment

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

LGTM

agjohnson
agjohnson reviewed Jul 23, 2020
View reviewed changes
Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for covering the doc updates as well!

@agjohnson agjohnson added this to the 0.5.1 milestone Jul 23, 2020
stsewd
stsewd approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

LGTM, we can update the changelog too https://github.com/readthedocs/sphinx_rtd_theme/blob/master/docs/changelog.rst

@stsewd
Copy link
Member

stsewd commented Oct 8, 2020

ha, found this #777, there we decided to create a block instead of another option. @agjohnson thoughts?

@Blendify
Copy link
Member

Blendify commented Oct 8, 2020

I would prefer to add a block here, this is a bit niche of a request. Plus a block is even more customizable.

@nevetS nevetS mentioned this pull request Nov 21, 2020
Open
@stsewd stsewd added Needed: design decision A core team decision is required Status: blocked Issue is blocked on another issue labels Dec 3, 2020
@Blendify
Copy link
Member

This will be supported natively in sphinx 4.0 so support there is no need to continue with this or. See sphinx-doc/sphinx#8737

@Blendify Blendify closed this Mar 11, 2021
@Blendify Blendify reopened this Mar 12, 2021
@Blendify
Copy link
Member

I realized this is a different feature than what I linked, I still think this should be a native sphinx feature rather than making it a theme option

@webknjaz
Copy link

I realized this is a different feature than what I linked, I still think this should be a native sphinx feature rather than making it a theme option

Exactly. Sphinx used to have a logo setting in conf.py, now they are renaming it to logo_url. It's a path to the image, nothing else.

benjaoming
benjaoming suggested changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@benjaoming benjaoming left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think it's important to rename the option before it gets more confusing :)

Otherwise LGTM, it's a relevant feature. Might want to suggest it upstream, i.e. for Sphinx's set of Jinja helper functions or for the basic theme.

docs/configuring.rst
@@ -126,6 +127,12 @@ Miscellaneous options
Only display the logo image, do not display the project name at the top of
the sidebar

.. confval:: logo_url
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. confval:: logo_url
.. confval:: logo_href

docs/configuring.rst
@@ -16,6 +16,7 @@ For example:
'canonical_url': '',
'analytics_id': 'UA-XXXXXXX-1', # Provided by Google in your dashboard
'logo_only': False,
'logo_url': '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'logo_url': '',
'logo_href': '',

Motivation: Since Sphinx 4.0, logo_url is used by Sphinx as the path for the logo image file. So this will be confusing.

sphinx_rtd_theme/layout.html
@@ -116,9 +116,9 @@
{% block sidebartitle %}

{% if logo and theme_logo_only %}
<a href="{{ pathto(master_doc) }}">
<a href="{{ theme_logo_url or pathto(master_doc) }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<a href="{{ theme_logo_url or pathto(master_doc) }}">
<a href="{{ theme_logo_href or pathto(master_doc) }}">

sphinx_rtd_theme/layout.html
{% else %}
<a href="{{ pathto(master_doc) }}" class="icon icon-home"> {{ project }}
<a href="{{ theme_logo_url or pathto(master_doc) }}" class="icon icon-home"> {{ project }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<a href="{{ theme_logo_url or pathto(master_doc) }}" class="icon icon-home"> {{ project }}
<a href="{{ theme_logo_href or pathto(master_doc) }}" class="icon icon-home"> {{ project }}

sphinx_rtd_theme/theme.conf
@@ -12,6 +12,7 @@ navigation_depth = 4
includehidden = True
titles_only =
logo_only =
logo_url =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logo_url =
logo_href =

@dnltz
Copy link

dnltz commented Mar 23, 2024

@benjaoming I'm actually interested in this change. Do you think I can open another PR with your requested changes included?

@benjaoming
Copy link
Contributor

@dnltz good question, @agjohnson will have to help you on that one 👍

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

@agjohnson agjohnson agjohnson left review comments

@benjaoming benjaoming benjaoming requested changes

@stsewd stsewd stsewd approved these changes

@JamesRandom JamesRandom JamesRandom approved these changes

Assignees
No one assigned
Labels
Needed: design decision A core team decision is required Status: blocked Issue is blocked on another issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: ability to customize logo link
8 participants
@simonw @stsewd @Blendify @webknjaz @dnltz @benjaoming @agjohnson @JamesRandom