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

MNT: more flexible static #1221

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Conversation

jklymak
Copy link
Contributor

@jklymak jklymak commented Mar 1, 2023

EDIT:

In mpl-sphinx-theme, we provide static/logo.svg which gets copied to _build/html/_static/logo.svg.

#1132 changed the behaviour of "logo" config option to do what sphinx's html_logo does, which is just copy the logo file from a relative path (relative to the conf.py) into _static, and raised a warning if the file did not exist. So:

html_theme_options = {
    "logo": {"link": "https://foo.bar/stable/",
             "image_light": "mylogos/wherever/logo2.svg",
             "image_dark": "mylogos/whereever/logo_dark.svg"},
...

would copy to _build/html/_static/logo2.svg, _build/html/_static/logo_dark.svg.

However, because we provide the logo as part of the theme payload, its "source" never exists - it gets copied directly into _static/logo2.svg. The PR here checks if that file exists, and doesn't warn if it already there. We specify that file in the conf.py as

"logo": {"link": "https://matplotlib.org/stable/",
             "image_light": "_static/logo2.svg",
             "image_dark": "_static/logo_dark.svg"},

which makes the links work properly in setup_logo_path. Unfortunately we cannot check for the existence of _build/html/_static/logo2.svg in setup_logo_path, because the build directory has not been populated yet.

@12rambau
Copy link
Collaborator

12rambau commented Mar 1, 2023

it's 1:30 at my place so way to late for a complete answer, I'll review the comments you made in the PR.
Food for thoughts if you want to continue digging while I sleep:
The purpose of #1132 is to handle logo whatever version of sphinx you are using: sphinx 5 (that have a logo jinja variable) and sphinx 6 (that have a logo_url variable and dropped logo).
By doing so we realized that the way we handle logo was not consistent with Sphinx so we updated as well (path now nned to be relative to conf instead of static)

@jklymak
Copy link
Contributor Author

jklymak commented Mar 1, 2023

OK, so I will try and make the path logic relative to conf (which its not with this change). But I think thats relatively easy while still allowing subdirectories.

@jklymak jklymak marked this pull request as draft March 1, 2023 00:37
@jklymak jklymak force-pushed the MNT-more-flexible-static branch from 24c7fe4 to 5d8e580 Compare March 1, 2023 03:41
@jklymak
Copy link
Contributor Author

jklymak commented Mar 1, 2023

OK, finally got to dig into this properly:

https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_logo

If given, this must be the name of an image file (path relative to the configuration directory) that is the logo of the docs, or URL that points an image file for the logo. It is placed at the top of the sidebar; its width should therefore not exceed 200 pixels. Default: None.

New in version 0.4.1: The image file will be copied to the _static directory of the output HTML, but only if the file does not already exist there.

So the change in 0.13.0 to "logo/image_light" seems in this spirit - the image Is copied to _build/html/_static and referenced from there.

Our difficulty, is that we would like the logo to be in the theme that inherits from pydata-sphinx-theme, and gets copied from theme/static/images to build/_static/images. From matplotlib's point of view, it's probably fine to move these up a level and then specify as _static/logo.svg. Then all that needs to happen is that copy_logo_images doesn't try to copy if the file exists. Thats what the newest push here does...

@choldgraf
Copy link
Collaborator

In case it's helpful, here how we set a default logo in the jupyterhub theme (which inherits from this theme)

https://github.com/jupyterhub/jupyterhub-sphinx-theme/blob/d4071da94ccb40216389c413c04a0bd684dc764d/src/jupyterhub_sphinx_theme/__init__.py#LL58C2-L58C2

I ran into the same problem you did, and my solution was just to be lazy and put the logo image directly under static, but if we could find a more principled way around this that would be great

src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
@jklymak jklymak force-pushed the MNT-more-flexible-static branch from 5d8e580 to 3041d0a Compare March 1, 2023 22:52
@jklymak
Copy link
Contributor Author

jklymak commented Mar 1, 2023

In case it's helpful, here how we set a default logo in the jupyterhub theme (which inherits from this theme)

yes, thats super helpful - we could probably do all the same defaulting for matplotlib and save all the client projects from specifying the logo at all.

@jklymak
Copy link
Contributor Author

jklymak commented Mar 1, 2023

but if we could find a more principled way around this that would be great

@choldgraf I guess your method of setting the path to the theme static and having pydata-sphinx-theme copy from there is fine. I feel the way proposed here is OK as well - let sphinx copy the theme static to build/html/_static and then let pydata-sphinx-theme only copy the file across if it needs to (or error if it can't find it). Certainly it will make our __init__.py less complex.

@jklymak jklymak marked this pull request as ready for review March 1, 2023 23:03
Copy link
Collaborator

@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.

This looks good to me - just one quick suggestion to make it clear when this might happen in the comments

@@ -1166,6 +1166,9 @@ def copy_logo_images(app: Sphinx, exception=None) -> None:
path_image = logo.get(f"image_{kind}")
if not path_image or isurl(path_image):
continue
if (staticdir / Path(path_image).name).exists():
# file already exists in static dir
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# file already exists in static dir
# file already exists in static dir
# e.g., because a theme has already bundled it there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slight edit, but general idea is the same:

            # file already exists in static dir e.g. because a theme has
            # bundled the logo and installed it there

@jklymak jklymak force-pushed the MNT-more-flexible-static branch from 3041d0a to df586d2 Compare March 5, 2023 18:12
@jklymak jklymak force-pushed the MNT-more-flexible-static branch from df586d2 to 988fde0 Compare March 5, 2023 18:12
Copy link
Collaborator

@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.

Looks good, thanks!

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 this pull request may close these issues.

3 participants