Opened 6 years ago
Closed 4 years ago
#46363 closed task (blessed) (fixed)
Images in about.php should be served locally, not from s.w.org
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Help/About | Keywords: | has-screenshots has-patch commit |
Focuses: | css, privacy | Cc: |
Description
As part of the ongoing efforts to reduce the external resources loaded by a vanilla WordPress install to zero, the images used in the about.php
page should be included in the ZIP archive instead of being served from https.//s.w.org/images
. Since these are SVG images, the download size should not increase significantly. Serving the files locally is beneficial both for reasons of privacy and speed.
Attachments (17)
Change History (38)
This ticket was mentioned in Slack in #core-privacy by pepe. View the logs.
6 years ago
#2
@
6 years ago
- Focuses performance removed
- Milestone changed from Awaiting Review to Future Release
- Type changed from defect (bug) to enhancement
- Version 5.1 deleted
This ticket was mentioned in Slack in #meta by garrett-eclipse. View the logs.
4 years ago
@
4 years ago
Alt patch to also include the freedoms.png main image even though it's not used in the css.
#6
@
4 years ago
- Keywords has-patch needs-testing has-screenshots added
- Milestone changed from Future Release to 5.6
- Owner set to garrett-eclipse
- Status changed from new to accepted
From a related discussion on meta Slack and #45395 let's try to get this in for 5.6.
I've uploaded the Freedoms image used in freedoms.php and updated the css to use it as found in 46363.diff. I also provided 46363.alt.diff in case we along with freedoms-2x.png want to include freedoms.png in core.
Currently with 5.5 the about.php only provides videos via CDN so we'll want to discuss them, they're probably too large to host with core. But maybe in 5.6 about page #51415 we can use gifs instead.
#7
@
4 years ago
- Keywords commit added; needs-testing removed
Tested. Yes, it serves locally as expected.
Committer note: The 2 images with this patch will need to be included in the src/wp-admin/images/
folder.
#9
in reply to:
↑ description
@
4 years ago
- Keywords has-patch commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to pputzer:
Since these are SVG images, the download size should not increase significantly.
Didn't want to block the commit, but the new freedoms-2x.png
image is ~460 KB, which is more than all the other images in wp-admin/images
together. Is there anything we can do to optimize it?
#11
@
4 years ago
For sure, aside from just optimizing the image we can revisit the breakpoints so we go first to two columns and then to single column. This means we won't need as large an image as the most space it'll fill should be reduced.
We could also break it out into 4 images but not sure if that is a benefit as their total weight would be the same but now they'd need 4 requests.
I'll explore the 2 column approach once Beta has landed here.
@
4 years ago
Initial patch for reducing image file size by switching to 2 columns until 420px and then going to single column leaving us with a 308px image meaning 2x is 616px (or 600 if we round down)
@
4 years ago
Original 1x image can now be used as the 2x image as we reduced the max image size by half. This is the original on s.w.org run through ImageOptim for a further improvement.
@
4 years ago
Showing the responsive approach dropping to two column before 1 column at a lower breakpoint means we can reduce the max size needed for the images.
@
4 years ago
Inspect illustration showing new max image size when at single column 420px screen wide.
#12
@
4 years ago
- Focuses css added
- Keywords has-patch needs-testing dev-feedback added
I've uploaded an initial patch 46363.2.diff to illustrate the concept I had on reducing the image size.
In fact in the freedoms.2.png has cut that size into a quarter (~460kb down to now ~108kb) by reducing the max image width into half it's original.
A few notes as this is only an initial patch and could be improved;
- I tweaked the existing
.has-4-columns
class to achieve this. I fear this is or will be used for another implementation where they wanted the original responsive behaviour. To resolve this we could maybe introduce a new.has-2-mid-columns
class to apply our styles specifically. - I changed the name from freedoms-2x.png to just freedoms.png in the patch, but the image I uploaded since there was another prior was renamed to freedoms.2.png so when downloading please rename.
- I introduced a new breakpoint at 420px after testing and found that kept the images when 2 column still looking nice and achieved the exact 1/2 of our original size allowing me to just use the pre-existing 1x version of the freedoms image off the CDN. Note: I only found that coincidence after I'd found a breakpoint I was happy with sizing on.
CC @SergeyBiryukov
#13
@
4 years ago
- Keywords needs-design-feedback needs-refresh added
Adding needs-design-feedback
to confirm the direction of using 2 columns on medium sizes and double-check the breakpoints. I've added needs-refresh
as mentioned in my last comment for back-compat I believe we'll want to introduce a new class instead of adding a breakpoint to the existing to avoid changing other sections.
This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.
4 years ago
@
4 years ago
Refresh to introduce a .med-2-columns
class to avoid any back-compat issues on changing the existing .has-4-columns
class.
#15
@
4 years ago
- Keywords dev-feedback needs-design-feedback needs-refresh removed
- Status changed from reopened to accepted
In 46363.3.diff I've refreshed the patch to ensure we don't introduce any back-compat issues by updating the .has-4-columns
class.
Aside from testing this is looking pretty good.
@
4 years ago
With patch 46363.3.diff, showing HTML with med-2-columns class attribute and image served locally.
#16
@
4 years ago
- Keywords commit added; needs-testing removed
Tested on Firefox and Safari. Results:
.med-2-columns
class attribute is applied- responsive styling behaves as expected
- image is served locally
This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.
4 years ago
#18
@
4 years ago
I think it would be safe to drop has-4-columns
to two columns at that middle size by default, rather than introducing a new class — I'm actually surprised it's not already doing that. I don't remember having any specific reason for dropping directly to a single column, and the 4-column design is usually only used for text (developer notes). (well, and here :) )
@
4 years ago
Refresh of the second diff to move into the 480 breakpoint. And avoid the .mid-2-columns that the third diff had. This is just awaiting a resized image.
#19
@
4 years ago
46363.5.diff iterates on the previous patch to fix the second row of items on IE, and uses the 480px breakpoint for switching to single column. I also got image resizing sorted out, so now this 800px wide freedoms.png
image (350 KB) can replace the freedoms-2x.png
(460 KB).
To summarize the patch itself, it introduces a breakpoint into the 4-column layout, so that we don't have a large jump from 4 columns down to a much wider single column at 782px. The 2 column layout exists from 782px to 480px, when it drops down to the single column. This means we can shrink the image used on the freedoms page, which saves about 110KB.
2x Freedoms image used in CSS