-
Notifications
You must be signed in to change notification settings - Fork 340
Make icon links and link shortening optional #2109
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
base: main
Are you sure you want to change the base?
Conversation
src/pydata_sphinx_theme/__init__.py
Outdated
f"type {type(theme_options.get('icon_links'))}." | ||
) | ||
if theme_options.get("icon_links") is not None: | ||
if not isinstance(theme_options.get("icon_links", []), list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why on current main
, setting icon_links = None
in your conf.py
would cause an error here. But if you don't want icon links, why not just leave icon_links
undefined? Genuinely curious if there's a reason you need this.
src/pydata_sphinx_theme/__init__.py
Outdated
}, | ||
) | ||
if icon_links is not None: | ||
for url, icon, name in shortcuts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as above: the code on main
is fine if icon_links
is not defined, it only errors if you explicitly set icon_links=None
in the html_theme_options
dictionary of conf.py
. So why do that?
Hey @westurner. This PR has been open for a while. I was wondering if you could get it over the finish line or if we should see if someone in the PST team can help get this wrapped up? Thanks for your contribution 🌻 |
Sorry, I don't mean to block. Anyone is welcome to take this |
Not a problem at all. I only wanted to check if you were planning to keep on working on this or if you needed some help. |
Hey @drammock, I pushed some commits to this PR to address your previous review. I poked around a bit at this PR, played with it against a personal Sphinx project, and like you, I couldn't find a good use case for handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks reasonable. Would be good to have a test that disables link shortening, and confirm that the link is untouched.
Co-authored-by: Daniel McCloy <[email protected]>
Good idea, I'll add a test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are passing, this should be ready for review now
@@ -844,7 +844,10 @@ def test_theme_switcher(sphinx_build_factory, file_regression) -> None: | |||
|
|||
def test_shorten_link(sphinx_build_factory, file_regression) -> None: | |||
"""Regression test for "edit on <provider>" link shortening.""" | |||
sphinx_build = sphinx_build_factory("base").build() | |||
confoverrides = { | |||
"html_theme_options": {"shorten_urls": True}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure why the default value in theme.conf isn't picked up by the test build process, but being forced to set shorten_urls
to true also makes the setting more explicity.
Backwards compatible:
Not backwards compatible:
dont_shorten_urls=True
instead ofshorten_urls=True