-
Notifications
You must be signed in to change notification settings - Fork 110
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
Integrate Auto Sizes with Image Prioritizer to ensure correct sizes=auto #1322
Integrate Auto Sizes with Image Prioritizer to ensure correct sizes=auto #1322
Conversation
a47655d
to
ed3e37c
Compare
2cf18cf
to
bec5704
Compare
6fef0b8
to
b9cadf9
Compare
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. |
ce07f44
to
9662ed6
Compare
9662ed6
to
74ac1bb
Compare
cf9584c
to
f7b4798
Compare
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Thanks for this, @westonruter. I assume that making use of OD functionality directly when enabled is needed because there is not a better way to make sure that the auto-sizes functionality runs late enough that any alterations that OD makes to the |
@joemcgill Here OD is being leveraged as a way to make modifications to the output-buffered page, which Image Prioritizer may have also modified to add/remove |
@westonruter, thanks for clarifying. I think there is a slight by important difference between: a) This plugin uses OD when available to more accurately apply I assumed that the goal of this was to address the latter, which is why I was asking about wether there is an alternate way that this plugin could account for any cases (including but not limited to OD) where the loading attribute is modified later than we currently apply |
@joemcgill actually it's both, isn't it? It does more accurately add |
@joemcgill I fixed the merge conflict. (Difficult to do on mobile!) |
Right now this PR is addressing both, but what I'm trying to better understand is what the primary objective of this PR is, since it isn't related to an open issue. While I see OD being very valuable tool in potentially fixing various types of bugs where third party plugins (like Image Prioritizer) modify the The two ways that I'd prefer to address the Image Prioritizer issue would be to either:
|
Oops. I failed to link this PR to the open issue. See #1099.
Since Image Prioritizer always applies changes to lazy-loading (either adding
OD is not made a dependency with this PR. Rather, it integrates with OD if it is active. I kept all the OD functionality in a separate file to ensure it doesn't trip up core merge.
The inclusion of |
|
||
$sizes = $context->processor->get_attribute( 'sizes' ); | ||
if ( ! is_string( $sizes ) || 'lazy' !== $context->processor->get_attribute( 'loading' ) ) { | ||
return false; |
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.
Oh, what if sizes=auto
had been added by Auto Sizes but Image Prioritizer removed loading=lazy
since the image actually appears in the initial viewport? Should this make sure that auto
is removed from sizes
as well? Is it bad if a non-lazy loaded image includes sizes=auto
?
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.
Addressed in f5e4231
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.
Tested the latest updates locally and everything looks good. I still have reservations about using OD to make late adjustments to the sizes
attribute, but this at least fixes the Image Prioritizer problem until we have a better option. Let's land it.
This integrates the Auto Sizes plugin with Image Prioritizer (and Optimization Detective) to ensure that
sizes=auto
is added to all of the images which actually end up lazy-loaded. Conversely, if Auto Sizes addedsizes=auto
to an initially lazy-loaded image, but then Image Prioritizer removedloading=lazy
since the image is actually in the initial viewport, then removesizes=auto
.Fixes #1099