Enable end user opt-in to generate all sizes in fallback format #1689
Conversation
…lity to use added new settings
b1ink0
requested review from
adamsilverstein,
felixarntz and
westonruter
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
@b1ink0 A bit of feedback here, but overall this looks great!
plugins/webp-uploads/helper.php
Outdated
| * | ||
| * @return bool True if the option is enabled, false otherwise. | ||
| */ | ||
| function webp_uploads_is_fallback_all_sizes_enabled(): bool { |
There was a problem hiding this comment.
Maybe a better name would be something like webp_uploads_should_generate_all_fallback_sizes()?
plugins/webp-uploads/hooks.php
Outdated
| $allowed_sizes[ $size ] = ! empty( $size_details['provide_additional_mime_types'] ); | ||
| if ( isset( $size_details['provide_additional_mime_types'] ) ) { | ||
| // Give priority to the 'provide_additional_mime_types' property. | ||
| $allowed_sizes[ $size ] = (bool) $size_details['provide_additional_mime_types']; | ||
| } else { | ||
| // If 'provide_additional_mime_types' is not set, use the fallback all sizes setting. | ||
| $allowed_sizes[ $size ] = webp_uploads_is_fallback_all_sizes_enabled(); | ||
| } |
There was a problem hiding this comment.
Rather than doing this here, I think a more elegant solution would be to decouple the UI control and option from the main logic, by using the webp_uploads_image_sizes_with_additional_mime_type_support filter below. So I think this would be better separately implemented.
It would also allow the code to be a bit more efficient: We should only need to call webp_uploads_is_fallback_all_sizes_enabled() once, not for every size available. If false we can immediately return without changing anything. If true, we go through the array of sizes and set them to true.
I think it's okay not to check the provide_additional_mime_types value directly to see if it's false. It's primary purpose it to enable (not disable) this behavior anyway, and since the setting explicitly says "all", it's fine to do it for all sizes, regardless of what other configuration may be present.
plugins/webp-uploads/hooks.php
Outdated
| if ( webp_uploads_is_fallback_all_sizes_enabled() ) { | ||
| echo '<meta name="generator" content="webp-uploads-fallback-all-sizes ' . esc_attr( WEBP_UPLOADS_VERSION ) . '">' . "\n"; | ||
| } |
There was a problem hiding this comment.
Why is that being added? We shouldn't have an extra generator tag for this feature specifically. If we need feature-specific indicators in the frontend, let's discuss that in a separate issue.
There was a problem hiding this comment.
In the #1513 there is mention of the adding the generator tag thats why I added it.
Ideally when users enable this feature we should notate that in the Generator tag so we can later discover how many sites opt into generating all image sizes.
Should I remove the generator tag code from this PR?
There was a problem hiding this comment.
Yeah, I think so, let's not do it in this PR, as it needs further discussion.
Maybe worth opening an issue to discuss whether/how we want to make more specific features discoverable in the frontend? Such an issue could link to that comment as one example reference.
plugins/webp-uploads/settings.php
Outdated
| // Add a setting to generate fallback images in all sizes including custom sizes. | ||
| register_setting( | ||
| 'media', | ||
| 'perflab_generate_fallback_all_sizes', |
There was a problem hiding this comment.
Maybe rename this to perflab_generate_all_fallback_sizes, that reads a bit better.
plugins/webp-uploads/settings.php
Outdated
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| function webp_uploads_generate_fallback_all_sizes_callback(): void { |
There was a problem hiding this comment.
See above in terms of naming.
plugins/webp-uploads/settings.php
Outdated
| // Add setting field for generating fallback images in all sizes including custom sizes. | ||
| add_settings_field( | ||
| 'perflab_generate_fallback_all_sizes', | ||
| __( 'Output all fallback images', 'webp-uploads' ), |
There was a problem hiding this comment.
| __( 'Output all fallback images', 'webp-uploads' ), | |
| __( 'Generate all fallback image sizes', 'webp-uploads' ), |
felixarntz
added
[Type] Enhancement
…ub.com/b1ink0/performance into add/option-to-generate-all-image-sizes
Summary
Fixes #1513
Relevant technical choices
This PR adds a checkbox users can check right below the "generate fallback images" checkbox (and it would only be enabled when "generate fallback images" checkbox is checked).
When users enable this feature it is notate in the Generator tag so we can later discover how many sites opt into generating all image sizes.
Settings Page Screenshots
Sample outputs for the below custom sizes
When the new option is off
When the new option is on