[Docs] Replace PNG/ICO images with SVG #2869

abravalheri · 2021-11-10T21:21:41Z

Motivation

As pointed out in #2853 (comment), the best police for usability and the git repo itself is to stick with vectorial images instead of binary files.

Initially I thought that Safari did not support SVG "favicons" but according to a recent article now it does, which make all the main desktop browsers compatible.

Summary of changes

Replaced png images included in the rst files with the equivalent svg
Added a "in-tree" Sphinx extension to allow (multiple) SVG "favicons"
Some modifications on the favicon.svg and logo-symbol-only.svg to better suite the usage.

Closes #2867

Pull Request Checklist

Changes have tests
News fragment added in changelog.d/.
(See documentation for details)

webknjaz

This in an interesting approach. LGTM but I've got a few questions.

Is the sphinx extension only needed to inject multiple favicon tags? Is the manual copying really necessary?
Have you considered applying something like https://jakearchibald.github.io/svgomg/ as an optimization? As a separate PR maybe.

abravalheri · 2021-11-10T23:15:57Z

Hi @webknjaz thank you very much for the review

Is the sphinx extension only needed to inject multiple favicon tags?

The extension is also needed to add the correct mime type.
According to the referenced article:

Your HTML page should have a <link> tag in its <head> with
rel="icon", type="image/svg+xml" and href with a link to 
SVG file as attributes.

If you inspect the current documentation page you can see that just by using the html_icon configuration variable, Sphinx omit that:

<link rel="shortcut icon" href="_static/favicon.ico">

Is the manual copying really necessary?

I was following the approach sphinx itself uses every time you add a .. image:: or .. picture::, which is also used for html_logo (however html_logo is copied to _static instead of _images).

I suppose this can be done in a different way, for example, by relying on _static, but then we have to separate images/README.rst from images/*.svg (from a few experiments I did, I have the impression Sphinx does not like .rst files in the _static path...)

Have you considered applying something like https://jakearchibald.github.io/svgomg/ as an optimization? As a separate PR maybe.

Yes no problem, I can investigate optimisers in a separate PR. However, I think it is good to leave the "*-editable-inkscape.svg" files unchanged right (so people have an easier time modifying them in the future if necessary)?

docs/_ext/_custom_icons.py

webknjaz · 2021-11-11T02:50:44Z

Is the sphinx extension only needed to inject multiple favicon tags?

The extension is also needed to add the correct mime type. According to the referenced article:
Your HTML page should have a <link> tag in its <head> with
rel="icon", type="image/svg+xml" and href with a link to 
SVG file as attributes.
If you inspect the current documentation page you can see that just by using the html_icon configuration variable, Sphinx omit that:
<link rel="shortcut icon" href="_static/favicon.ico">

FWIW I have an example where this works for me natively without specifying the type: https://ansible-pylibssh.rtfd.io. Although, I haven't checked whether it's cross-browser and it sounds like what you're doing is right.

OTOH the right thing to do, as I see it, would be to post an issue (and possibly a solution) on the Sphinx's tracker to avoid maintaining an in-tree extension. Until that happens, it's fine to have it here, of course. Just trying to look out for potential maintenance burden.

Is the manual copying really necessary?

I was following the approach sphinx itself uses every time you add a .. image:: or .. picture::, which is also used for html_logo (however html_logo is copied to _static instead of _images).

I suppose this can be done in a different way, for example, by relying on _static, but then we have to separate images/README.rst from images/*.svg (from a few experiments I did, I have the impression Sphinx does not like .rst files in the _static path...)

Fair enough. I just seem to recall that there were cases where Sphinx would copy stuff on its own, automatically. Although the proposal above would probably sort this out.

I've checked what I did in the past and it looks like I've just put the image under _static/ in the repo and referred to it from html_icon: https://github.com/ansible-community/ansible-lint/blob/24e83a1/docs/conf.py#L239.

Have you considered applying something like jakearchibald.github.io/svgomg as an optimization? As a separate PR maybe.

Yes no problem, I can investigate optimisers in a separate PR. However, I think it is good to leave the "*-editable-inkscape.svg" files unchanged right (so people have an easier time modifying them in the future if necessary)?

Yeah, maybe run SVGO during the build time even (although this would mean depending on NodeJS or dukpy).

webknjaz · 2021-11-11T02:52:47Z

docs/conf.py

+    {  # Version with thicker strokes for better visibility at smaller sizes
+        "rel": "icon",
+        "type": "image/svg+xml",
+        "file": "images/favicon.svg",


Maybe call the file properly instead of having a comment only in one place?

Hi @webknjaz, I am not sure if I understood correctly your comment.
Do you mean having a "relative_path" instead of "file" in the dictionary? That would make sense...

No, I'm talking about the filename. It's not cool that the file is called favicon.svg and needs a code comment above explaining what it is. This information should be present in a file name, treat it as a variable in code. If the name is self-explanatory, then it'd be useful when checking out the HTML output, or just opening the URL in a browser, or while browsing the repository. A name like favicon does not give any hint about what the difference is from the other SVG file, nor it points at the relation between this and logo-symbol-only.svg.
You've added two code comments for each entry. The first one is quite useful because it explains the motivation but this second one has both useful and pointless bits. If a code comment has to include explanations about what the things are, you're probably calling those things wrong names.

webknjaz · 2021-11-11T02:55:03Z

docs/conf.py

@@ -101,8 +104,7 @@

 # HTML theme
 html_theme = 'furo'
-html_logo = "images/logo.png"
-html_favicon = "images/favicon.ico"


Maybe keep it pointing at SVG as a fallback?

Once we agree in a good file structure, I can test this.
I suppose it will work fine, although it might obfuscate the setup for multiple sizes in some browser.

docs/_ext/_custom_icons.py

abravalheri · 2021-11-11T09:57:18Z

Thank you for the comments guys. I will try to address some of those later today.

FWIW I have an example where this works for me natively without specifying the type: https://ansible-pylibssh.rtfd.io. Although, I haven't checked whether it's cross-browser and it sounds like what you're doing is right.

That is the tricky part isn't it? 😄
I did some tests in a 3 or 4 browsers on the OSs I have access to, but it is hard to guarantee it will work everywhere, that is why I am trusting the information in the reference.

If the project is OK with having just one SVG icon via the default Sphinx mechanism, please let me know and I will change the PR accordingly.

In that scenario, which one would you prefer:

the version that is better for small sizes
the exactly the same logo as the displayed in the sidebar (except from the text)

(The small version may appear different from the logo when enlarged -- I suppose that can happen when creating shortcuts, pinning to the start-page/speed dial, etc).

(The standard logo is less visible/more difficult to be seen in the browsers tab-bar)

Fair enough. I just seem to recall that there were cases where Sphinx would copy stuff on its own, automatically. Although the proposal above would probably sort this out.
I've checked what I did in the past and it looks like I've just put the image under _static

Yes, Sphinx will automatically copy everything inside all the directories listed in the html_static_path (["_static"] by default) into the _static directory of the build.
I suppose that is your preferred approach, so I will proceed with this change in the next commits.

webknjaz · 2021-11-11T12:59:46Z

Thank you for the comments guys. I will try to address some of those later today.

FWIW I have an example where this works for me natively without specifying the type: ansible-pylibssh.rtfd.io. Although, I haven't checked whether it's cross-browser and it sounds like what you're doing is right.

That is the tricky part isn't it? smile I did some tests in a 3 or 4 browsers on the OSs I have access to, but it is hard to guarantee it will work everywhere, that is why I am trusting the information in the reference.

Yep, this is why I said that you're doing the right thing :)

If the project is OK with having just one SVG icon via the default Sphinx mechanism, please let me know and I will change the PR accordingly.

I actually like having fallbacks. So keep them.

Fair enough. I just seem to recall that there were cases where Sphinx would copy stuff on its own, automatically. Although the proposal above would probably sort this out.
I've checked what I did in the past and it looks like I've just put the image under _static

Yes, Sphinx will automatically copy everything inside all the directories listed in the html_static_path (["_static"] by default) into the _static directory of the build. I suppose that is your preferred approach, so I will proceed with this change in the next commits.

Well, I was thinking about how Sphinx special-cases some of these settings like html_logo and html_favicon. I've just made use of html_logo = html_favicon = '_static/<img>.svg for ansible-lint project and it worked like a charm: https://ansible-lint--1760.org.readthedocs.build/en/1760/.

abravalheri · 2021-11-11T14:34:55Z

Well, I was thinking about how Sphinx special-cases some of these settings like html_logo and html_favicon. I've just made use of html_logo = html_favicon = '_static/<img>.svg for ansible-lint project and it worked like a charm: https://ansible-lint--1760.org.readthedocs.build/en/1760/.

The example that you mentioned is very interesting. I believe that what is happening in the ansible-lint code is a coincidence actually... According to the docs:

html_favicon

If given, this must be the name of an image file (path relative to the configuration directory)
that is the favicon of the docs, or URL that points an image file for the favicon.
...
New in version 0.4: 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 you could have the file in any folder relative to the conf.py directory and it would be copied to the _static file in the output.

abravalheri · 2021-11-11T14:50:46Z

@webknjaz, would the following organisation be fine?

docs
├── ...
├── _static
│   ├── icon-small.svg  # favicon renamed to be more clear
│   └── icon.svg        # the standard icon for the project (logo without the text)
└── images
    ├── README.rst
    ├── banner-640x320.svg
    ├── ...  # several SVG files available to be used by the project
    ├── ...  # no `logo-symbol-only.svg` since it is redundant with `_static/icon.svg`
    └── logo.svg

Do you have a different suggestion?

Notes:

By placing the icons in the _static we can avoid copying them in the Sphinx extension.
Simply renaming images => _static or adding images to html_static_path cannot be done because of the README.rst file (in that case we need to move that file to another location).
The idea of having a images folder with a README was introduced in Adopt a logo #2227. I have no strong feelings about it and I am happy to adapt to different suggestions.

This change has been previously proposed and agreed upon in pypa#2869. The idea here is to avoid the burden of maintaining an in-tree sphinx extension, now that `sphinx-favicon` is available and covers setuptools use-case.

In this commit the SVG images (logo, banners, etc) were optimised with the help of https://pypi.org/project/scour/. This change has been previously requested/discussed and agreed upon on pypa#2869. The `*editable-inkscape.svg` files are preserved in the original form.

abravalheri added 5 commits November 10, 2021 18:49

Remove binary image files

900d613

Use SVG images instead of PNG

b19cdba

Add SVG favicons via custom Sphinx extension

0c7a5b9

Add dark mode to favicon

7ec2d78

Add news fragment about change to SVG images only

e18db4b

abravalheri marked this pull request as ready for review November 10, 2021 21:58

webknjaz approved these changes Nov 10, 2021

View reviewed changes

webknjaz added documentation enhancement trivial labels Nov 10, 2021

jaraco reviewed Nov 11, 2021

View reviewed changes

docs/_ext/_custom_icons.py Show resolved Hide resolved

webknjaz reviewed Nov 11, 2021

View reviewed changes

jaraco approved these changes Nov 12, 2021

View reviewed changes

Merge branch 'main' into prefer-svg-images

4b980ef

jaraco merged commit 3f9bbce into pypa:main Nov 14, 2021

abravalheri mentioned this pull request Nov 17, 2021

Improve favicon support by incorporating sphinx-favicon in sphix.ext sphinx-doc/sphinx#9861

Open

abravalheri mentioned this pull request Jan 6, 2022

[Docs] Use sphinx-favicon instead of custom in-tree extension #3008

Merged

2 tasks

abravalheri deleted the prefer-svg-images branch January 6, 2022 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Docs] Replace PNG/ICO images with SVG #2869

[Docs] Replace PNG/ICO images with SVG #2869

abravalheri commented Nov 10, 2021

webknjaz left a comment

abravalheri commented Nov 10, 2021 •

edited

webknjaz commented Nov 11, 2021

webknjaz Nov 11, 2021

abravalheri Nov 11, 2021

webknjaz Nov 11, 2021

webknjaz Nov 11, 2021

abravalheri Nov 11, 2021

abravalheri commented Nov 11, 2021

webknjaz commented Nov 11, 2021

abravalheri commented Nov 11, 2021 •

edited

abravalheri commented Nov 11, 2021 •

edited

[Docs] Replace PNG/ICO images with SVG #2869

[Docs] Replace PNG/ICO images with SVG #2869

Conversation

abravalheri commented Nov 10, 2021

Motivation

Summary of changes

Pull Request Checklist

webknjaz left a comment

Choose a reason for hiding this comment

abravalheri commented Nov 10, 2021 • edited

webknjaz commented Nov 11, 2021

webknjaz Nov 11, 2021

Choose a reason for hiding this comment

abravalheri Nov 11, 2021

Choose a reason for hiding this comment

webknjaz Nov 11, 2021

Choose a reason for hiding this comment

webknjaz Nov 11, 2021

Choose a reason for hiding this comment

abravalheri Nov 11, 2021

Choose a reason for hiding this comment

abravalheri commented Nov 11, 2021

webknjaz commented Nov 11, 2021

abravalheri commented Nov 11, 2021 • edited

abravalheri commented Nov 11, 2021 • edited

abravalheri commented Nov 10, 2021 •

edited

abravalheri commented Nov 11, 2021 •

edited

abravalheri commented Nov 11, 2021 •

edited