-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
Update Sphinx config #898
Update Sphinx config #898
Conversation
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 |
What's needed is for the CLA bot to work. I ran into this on another issue a few weeks ago. |
@terryjreedy Fixing edgedb/cla-bot#50 , at least for the specific issue @AA-Turner is running in to... |
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.
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):
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?
(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. |
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 Also, nitpick but just curious—is there a reason the CSS filename uses underscores, while everything else uses hyphens? |
What settings did you use? I can perceive a loss of quality (on a Retina screen), this one has more jagged edges:
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/ |
I just used ImageMagick's 'mogrify' command with '-antialias', I didn't realise we were optimising for filesize! A |
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 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
Oh, well that was @ezio-melotti 's stated reason for requesting this change,
so I figured if we were doing it, we may as well get the benefit of it. |
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.
The PR seems to include both images now (original and 128x128). Do we still need both?
AFAIK, per our discussion it should just include the optimized SVG, which will work lossleslly for all sizes. |
Switched to vector logo, I don't think there are any more outstanding points? A |
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.
Thanks!
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.
LGTM, thanks @AA-Turner !
Related to #895
conf.py
file, removing outdated comments and reformatting / re-grouping settingsrstlint.py
(we now usesphinx-lint
)cc @ezio-melotti
A