FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Update Sphinx config by AA-Turner · Pull Request #898 · python/devguide · GitHub

Update Sphinx config - #898

Merged
ezio-melotti merged 18 commits into
python:mainfrom
AA-Turner:update-conf-py
Jul 12, 2022
Merged

Update Sphinx config#898
ezio-melotti merged 18 commits into
python:mainfrom
AA-Turner:update-conf-py

Conversation

Copy link
Copy Markdown
Member

Related to #895

  • Reformats the conf.py file, removing outdated comments and reformatting / re-grouping settings
  • Removes old templates unused by Furo
  • Removes old stylesheets unused by Furo
  • Removes rstlint.py (we now use sphinx-lint)
  • Makes the site logo smaller to fit more headings in the sidebar

cc @ezio-melotti

A

ghost commented Jun 15, 2022
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

AA-Turner added the type-feature Additions; New content or section needed label Jun 16, 2022
AA-Turner self-assigned this Jun 16, 2022

Copy link
Copy Markdown
Member Author

Due to the CLA thing this PR will need to be merged by a repo admin, as we've done on the PEPs repo (explanatory comment).

A

Copy link
Copy Markdown
Member

What's needed is for the CLA bot to work. I ran into this on another issue a few weeks ago.

Copy link
Copy Markdown
Member

@terryjreedy Fixing geldata/cla-bot#50 , at least for the specific issue @AA-Turner is running in to...

CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A quick check of the preview reveals that the logo doesn't show up at all now because of a typo in the path (see suggestion to fix).

It does look a lot better when I simulated the change on the existing devguide + the new directory layout (though its less critical, given the new layout makes the sidebar a lot shorter):

Preview screenshot

Also, while I like the descriptiveness, giving the custom CSS stylesheet a change-specific names seems to create the future expectation of creating a whole new injected CSS stylesheet for each discrete change, which is pretty inefficient. Could we maybe just name the file custom_styles.css or similar, to leave room for any future local customizations without requiring a rename?

Comment thread conf.py Outdated

CAM-Gerlach commented Jun 18, 2022
edited
Loading

Copy link
Copy Markdown
Member

(Oops, somehow the comment didn't get submitted with the review)

EDIT: Since I can't dismiss/re-request the approving review or add a request-changes review mark as draft (though the approach I suggest in python/core-workflow#460 could enable that, if we want it), I've added the do-not-merge tag as an additional reminder against merging until the issue above is fixed.

CAM-Gerlach requested a review from ezio-melotti June 18, 2022 01:11
CAM-Gerlach added the DO-NOT-MERGE Do not merge this PR label Jun 18, 2022
Comment thread _static/adjust_logo_size.css Outdated
AA-Turner removed the DO-NOT-MERGE Do not merge this PR label Jun 19, 2022

CAM-Gerlach commented Jun 19, 2022
edited
Loading

Copy link
Copy Markdown
Member

Due to inefficient downsampling and compression, the 128 x 128 version in the PR was barely any smaller than the original (7.8 KB vs 9.7 KB) despite having 16 times fewer pixels. With a more efficient downsampling strategy (nearest neighbor), using a 8-bit palette (like the original) and running optipng with optimal settings, this by be reduced by a further 3x to only 2.6 KB with no loss of perceptible quality; see attached.

python-logo-128x128_2.zip

Also, nitpick but just curious—is there a reason the CSS filename uses underscores, while everything else uses hyphens?

hugovk commented Jun 20, 2022

Copy link
Copy Markdown
Member

Due to inefficient downsampling and compression, the 128 x 128 version in the PR was barely any smaller than the original (7.8 KB vs 9.7 KB) despite having 16 times fewer pixels. With a more efficient downsampling strategy (nearest neighbor), using a 8-bit palette (like the original) and running optipng with optimal settings, this by be reduced by a further 3x to only 2.6 KB with no loss of perceptible quality; see attached.

python-logo-128x128_2.zip

What settings did you use?

I can perceive a loss of quality (on a Retina screen), this one has more jagged edges:

Original This one

Screenshot:

Original:

This one:

Actually, let's use an SVG instead. The resized one has a bit fuzzy edges. The SVG has much cleaner lines, like the unresized PNG currently at https://devguide.python.org/

https://commons.wikimedia.org/wiki/File:Python-logo-notext.svg is 5.37 KB, and squashes down 40.83% to 2.19k with https://jakearchibald.github.io/svgomg/

Copy link
Copy Markdown
Member Author

I just used ImageMagick's 'mogrify' command with '-antialias', I didn't realise we were optimising for filesize!

A

Copy link
Copy Markdown
Member

What settings did you use?

I can perceive a loss of quality (on a Retina screen), this one has more jagged edges:

That's because of the nearest neighbor resampling strategy, which does result in some loss of quality along edges (which wasn't too perceptible on my non-hiDPI 1200p monitor unless I was looking carefully for it) and can cause more serious issues in areas of dense, high-contrast detail (particularly small text, such as in a screenshot), but can dramatically reduce file size by reducing color consistency and ballooning the total number of colors in the palette caused by interpolation-based strategies like bicubic and Lanczos, which appeared to be the main culprit behind the file size barely decreasing when downscaled by 16x here.

However, it's typically better to downscale from the original PNG/SVG, or just use the optimized SVG directly, as it results in no loss of quality (if not higher quality on HiDPI displays) with a similar or smaller filesize to the PNG -> PNG downscale using the aggressive nearest-neighbor strategy, I wasn't able to find a SVG of just the Python logo on the official Python logo page, and was too lazy didn't want to crop it manually and not have it come out the same, so I didn't go ahead with that. But seeing as there is one, and it actually optimizes smaller than even the most aggressive PNG while displaying better, using the optimized SVG as you suggest seems to be a no-brainer.

Additionally, I note that the nominal size of that SVG is 110px x 109px, which it should default to (unless the original theme stylesheet is using width: 100% instead of max-wdith: 100%), which is actually closer to the size that we originally wanted. Therefore, we might not actually need the stylesheet, though as @hugovk mentions it may improve rendering performance and avoid relayouts when rendering since it gives the browser the exact image size before downloading it.

I didn't realise we were optimising for filesize!

Oh, well that was @ezio-melotti 's stated reason for requesting this change,

By resizing it, the page will load faster because it will have to download a smaller image

so I figured if we were doing it, we may as well get the benefit of it.

ezio-melotti left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The PR seems to include both images now (original and 128x128). Do we still need both?

Copy link
Copy Markdown
Member

AFAIK, per our discussion it should just include the optimized SVG, which will work lossleslly for all sizes.

Copy link
Copy Markdown
Member Author

Switched to vector logo, I don't think there are any more outstanding points?

A

hugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks @AA-Turner !

Comment thread conf.py Outdated
ezio-melotti merged commit 2bbbdca into python:main Jul 12, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature Additions; New content or section needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL