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 marked this pull request as draft February 1, 2022 17:00
@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 February 4, 2022 12:38
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

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

@mitogh mitogh added [Focus] Site Health [Type] Feature A new feature within an existing module labels Feb 7, 2022
@mmuyskens
Copy link

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

@adamsilverstein
Copy link
Member

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

@adamsilverstein
Copy link
Member

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
Copy link
Member Author

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

@adamsilverstein adamsilverstein added this to the 1.0.0-beta.1 milestone Feb 22, 2022
@adamsilverstein
Copy link
Member

@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

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

@adamsilverstein
Copy link
Member

👍🏼 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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