-
Notifications
You must be signed in to change notification settings - Fork 102
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
New check: article/images (location, resolution and filesize) #4594
Comments
Nathan also noticed that Ojuju had a massive 1.5mb .gif. These should be converted to mp4. |
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue fonttools#4594)
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue fonttools#4594)
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue fonttools#4594)
Here's a list of image resolutions sampled from the google/fonts repo and sorted (largest first). Where should we put the max resolution threshold?
|
@m4rc1e, I've renamed the issue to focus on this specific article/images new check proposal. For additional check proposals, please open separate issues. |
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue fonttools#4594)
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue fonttools#4594)
@nathan-williams do we care about resolution or just size? |
Given the example of Roboto Flex above, yes, I think we should have a max resolution rule. |
The purpose of this check is to ensure images (either raster or vector files) are placed on the correct place (an `images` subdirectory inside `article`) and they they are not excessively large in filesize and resolution. com.google.fonts/check/article/images Added to the Google Fonts profile. (issue #4594)
I've merged the PR with the new check implementation so that people can try it (it is marked as an experimental check). But I'll keep this issue open for additional discussion on proposed checking criteria. I've set the experimental check to:
|
Currently, we do not use an Will there be a limit on the number of images? Therefore, what is the maximum weight limit for the sum of all images? |
Thank you for the summary, @felipesanches. All of that looks good to me. @vv-monsalve I don't think we need a check for total image load at this time. As for the |
Okay. In that case, there will also be no limit on the number of images, right? About the limit for vector Vs. raster images. Couldn't it be the opposite, bigger for raster and smaller for vector? Edit: below are the values that we have in the GF-Guide, which would make more sense for each format.
|
I agree with @vv-monsalve It seems that we should accept larger rasters, rather than larger vectors.
But it could be useful to at least WARN, so that we could slightly nudge the whole library towards some consistency |
I can add more context here. The |
Thanks, @nathan-williams, for the added info. However, this value is the opposite of what you specified in the chat where you said 1,75 were for vector images. Could you confirm it is 1.75MB for the above formats? and in that case 800 kb for VECTORS? |
Also, add a bit more context on the rationale text. com.google.fonts/check/article/images On the Google Fonts profile (issue fonttools#4594)
Also, add a bit more context on the rationale text. com.google.fonts/check/article/images On the Google Fonts profile (issue #4594)
@nathan-williams Could you please check whether it is actually VECTORS that have the 800kb limit? We're suspecting that the numbers may be mixed, because vectors are usually smaller than raster images. |
Nate was/is out this week so I'll follow up with him on Monday :) Wow yes, that Roboto Flex hero.png is 3840 × 2160 and I made a few and in macOS Chrome Inspector I found the image is only rendered just under 900px wide anyway due the page layout, which came it at 900kb, but a JPEG (80% compression) version of the same resolution is ~166kb optimized:
So... probably we should have a "gf content" profile that has a dependency on imagemagick (or sips if running on macOS, a proprietary image converter) to empirically test all the images in the repo, convert them to/from png/jpg and test if the image compression format makes a big difference, and rescale the article images to 900px width proportionally. |
From Felipe's list of resolutions, I looked up the corresponding list of file sizes currently on main in PNG format, and ran a couple of simple conversions to JPEG, resized to 800px wide, to 1,200px wide, and without a resizing; I put the data in a Google Sheet, and here it is raw:
As you can see, Resizing some images doesn't make any difference as they don't have any sharp shapes (like small text labels) but for some the JPEG format makes it overall a little fuzzy even without resizing. Its only ~80 families so I'll manually review visually and make a PR to update them as needed. |
@felipesanches though surprising, the thresholds should indeed be set that way today. These are inferred constraints based on where we encounter timeouts in the processing infrastructure. I'm interested in adjusting timeouts where we can to shift those thresholds, but we aren't going to pursue that right now. |
@nathan-williams, thanks for the clarification! |
The initial implementation of this check will be included in today's 0.12.0 release. And any further improvements can be made for the v0.12.1 later this month. |
@felipesanches I think this can be closed then? :) |
On the Google Fonts profile, after being marked as **experimental** for 4 months since the 0.12.0a5 release. com.google.fonts/check/article/images (issue #4594)
On the Google Fonts profile, after being marked as **experimental** for 4 months since the 0.12.0a5 release. com.google.fonts/check/article/images (issue #4594)
We currently don't have any checks for family articles. I'm opening this issue so we can discuss possible checks we'd like to include in our CI for families that have articles.
The following have been proposed:
/images
directory@nathan-williams for the
/images
dir. Do we expect it in/ofl/family/article/images
or/ofl/family/images
?cc @chrissimpkins @davelab6 @emmamarichal @vv-monsalve @nathan-williams
The text was updated successfully, but these errors were encountered: