-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add WebP support in site health #141
Add WebP support in site health #141
Conversation
it's nice to see the output But most users won't know what to look for |
@pbearne Can you look at the screenshot again? Earlier I added a screenshot in the last line to indicate where I copied the code of checking WebP support from, but I saw how it can be mistaken as the output of the code, so I have uploaded the actual screenshot of output. Do you still feel we need to show percentage-wise support? I think now since the user has a clear picture to see that they have an action to take here, I feel we don't need to show partial support indication. |
Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>
…tangajjar/performance into feature/130-webp-support-site-health
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.
This looks good to me, let's wait on other people to chime in case they have any additional feedback.
Thank you for putting this together 🥇 🙇
I wouldn't call it critical. Kick it down to recommendations. |
Nice work @kirtangajjar - code looks good, I'll give this a test. |
This worked well in my testing! After testing though, I feel like the notice belongs under "recommended improvements" instead of "critical issues". Having WebP support is nice to have, it isn't really critical. |
Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
@adamsilverstein Done. LMK if you find any additional changes. |
Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
@kirtangajjar I left one additional tiny suggestion on the text, then we can merge this. Thanks for your work here! |
@adamsilverstein I've committed your suggestion. Thanks for giving it a second look! |
👍🏼 Looks good, nice work @kirtangajjar - going to go ahead and merge this. We'll get another chance to review the exact wording when we propose this feature for core. |
Summary
Fixes #130.
Relevant technical choices
Adds a module in site health to check for WebP support and shows a warning if support is not present.
If either GD or Imagick has WebP support, the WebP support warning is not shown.
The code of checking support in GD and Imagick is same as how it's shown in site health info's media handling section.
Screenshots