close
The Wayback Machine - https://web.archive.org/web/20221024153516/https://github.com/WordPress/performance/pull/141
Skip to content
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

Conversation

kirtangajjar
Copy link
Member

@kirtangajjar kirtangajjar commented Feb 1, 2022

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

Screenshot from 2022-02-03 18-19-09

@kirtangajjar kirtangajjar requested a review from audrasjb as a code owner Feb 1, 2022
@kirtangajjar kirtangajjar marked this pull request as draft Feb 1, 2022
@kirtangajjar kirtangajjar changed the title WIP Add WebP support in site health [WIP] Add WebP support in site health Feb 1, 2022
@pbearne
Copy link
Contributor

pbearne commented Feb 1, 2022

it's nice to see the output But most users won't know what to look for
I feel we need to add a "score" as to how good the support is.
ie a nice green 100% support if all is good and messages, if support expected/desired, is missing

@kirtangajjar
Copy link
Member Author

kirtangajjar commented Feb 3, 2022

@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.

@kirtangajjar kirtangajjar changed the title [WIP] Add WebP support in site health Add WebP support in site health Feb 4, 2022
@kirtangajjar kirtangajjar marked this pull request as ready for review Feb 4, 2022
modules/site-health/webp-uploads-enabled/load.php Outdated Show resolved Hide resolved
modules/site-health/webp-uploads-enabled/load.php Outdated Show resolved Hide resolved
modules/site-health/webp-uploads-enabled/load.php Outdated Show resolved Hide resolved
modules/site-health/webp-uploads-enabled/load.php Outdated Show resolved Hide resolved
Copy link
Member

@mitogh mitogh left a comment

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 🥇 🙇

@mitogh mitogh added [Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Type] Feature A new feature within an existing module labels Feb 7, 2022
@eclarke1 eclarke1 added the Needs Review Anything that requires code review label Feb 8, 2022
@mmuyskens
Copy link

mmuyskens commented Feb 8, 2022

I wouldn't call it critical. Kick it down to recommendations.

@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 11, 2022

Nice work @kirtangajjar - code looks good, I'll give this a test.

@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 11, 2022

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.

kirtangajjar and others added 2 commits Feb 14, 2022
Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
@kirtangajjar
Copy link
Member Author

kirtangajjar commented Feb 14, 2022

@adamsilverstein Done. LMK if you find any additional changes.

mitogh
mitogh approved these changes Feb 16, 2022
@adamsilverstein adamsilverstein added this to the 1.0.0-beta.1 milestone Feb 22, 2022
Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 22, 2022

@kirtangajjar I left one additional tiny suggestion on the text, then we can merge this. Thanks for your work here!

@kirtangajjar
Copy link
Member Author

kirtangajjar commented Feb 22, 2022

@adamsilverstein I've committed your suggestion. Thanks for giving it a second look!

@kirtangajjar kirtangajjar requested a review from mitogh Feb 23, 2022
@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 24, 2022

👍🏼 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.

@adamsilverstein adamsilverstein merged commit bd87192 into WordPress:trunk Feb 24, 2022
10 checks passed
@eclarke1 eclarke1 removed the Needs Review Anything that requires code review label Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose WebP support under Site Health status tab
6 participants