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: Auto Sizes for Lazy-loaded Images #904

Merged
merged 42 commits into from
Feb 19, 2024

Conversation

joemcgill
Copy link
Member

@joemcgill joemcgill commented Dec 14, 2023

Summary

Blocked for merge by #934.

This adds a new module that will automatically enhance the sizes attribute of lazy-loaded images to support the new auto syntax.

See: whatwg/html#4654

Fixes: #791.

Relevant technical choices

This adds just the basic functionality needed to implement auto sizes for images created by WP wp_get_attachment_image function or which get responsive attributes added by the wp_filter_content_tags callback. In both cases, auto is prepended to the sizes attribute only if the image already has loading="lazy" applied, which is a requirement to avoid introducing regressions. For more details about compatibility with browser implementations, see this comment.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

This adds a new module that will automatically enhance the `sizes` attribute of lazy-loaded images to support the new `auto` syntax.

See: whatwg/html#4654

Fixes: #791.
@joemcgill joemcgill added [Focus] Images no milestone PRs that do not have a defined milestone for release labels Dec 14, 2023
@joemcgill
Copy link
Member Author

Still need to add some test coverage, but wanted to share an initial implementation for testing/feedback.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joemcgill, code looks great so far.

One consideration here unrelated to the code itself is how we are going to publish this. A few thoughts:

  • We're right now in the last steps of unbundling modules (to be released mid-January). So we probably don't want to add any modules now.
  • For that reason I'd recommend to change this PR to go against a new feature branch for this module for now. That way development can continue, and once the repository has shifted to the full standalone plugins monorepo (likely early February) it can be merged into trunk.
  • Separately, we may want to already kick off the plugin review process for this? Even if the code as of now isn't necessarily final, it's probably solid enough to submit for review. You could use another experimental or local branch based on this where you add this new module to plugins.json. With that, you can then run npm run build-plugins and get the plugin in a version ready to submit to .org for review.
  • The only other thing needed for .org submission is a readme.txt (which we'll need later anyway). Could you maybe add one to this PR? You can probably copy one of the other standalone plugins' readme files and adjust just the few necessary bits.

modules/images/auto-sizes/load.php Outdated Show resolved Hide resolved
modules/images/auto-sizes/load.php Outdated Show resolved Hide resolved
@joemcgill
Copy link
Member Author

Thanks @felixarntz. Great questions about how we would publish this. This is already essentially the feature branch for this effort, so happy to just keep iterating here and/or submitting additional PRs into this branch. I'll address the rest of your suggestions over the coming week.

The bigger question you raise is how we make modules like this available for people to test before they're published to the wp.org repository once the unbundling effort is complete, given that size of the review backlog at the moment.

I wonder if we can keep some modules in an experimental state where they can be activated from the main performance lab plugin prior to being either promoted to a standalone plugin or as a direct implementation into Core. Right now, you can install the plugin from this repo and checkout this feature branch to test the functionality, but that wont work for modules added after the unbundling effort is complete.

Otherwise, I think we need to update the release script so that zips of pre-release plugins can be downloaded from the https://github.com/WordPress/performance/releases page so we're not completely dependent on the WP.org repo for distributing standalone plugins from this mono-repo.

@felixarntz
Copy link
Member

felixarntz commented Dec 18, 2023

@joemcgill

This is already essentially the feature branch for this effort, so happy to just keep iterating here and/or submitting additional PRs into this branch. I'll address the rest of your suggestions over the coming week.

Happy for you to keep working in here, though the main reason I'd suggest a feature branch is to avoid accidental merge into trunk. As in: Even once this has 2 approvals, we need to determine the best path forward before "just merging". Alternatively, one of us can just keep a "requesting changes" on this to avoid merging "by accident".

I wonder if we can keep some modules in an experimental state where they can be activated from the main performance lab plugin prior to being either promoted to a standalone plugin or as a direct implementation into Core. Right now, you can install the plugin from this repo and checkout this feature branch to test the functionality, but that wont work for modules added after the unbundling effort is complete.

Those are good ideas. We could always consider keeping experimental modules in Performance Lab in the beginning, and once they're approved on .org, we'd phase them out. In that case, maybe the migration logic that @10upsimon and @mukeshpanchal27 have been working on should remain in the plugin even indefinitely, as it would continue to be useful in the future, not only this initial migration push as previously assumed.

@joemcgill joemcgill marked this pull request as draft December 18, 2023 18:58
@joemcgill
Copy link
Member Author

Even once this has 2 approvals, we need to determine the best path forward before "just merging". Alternatively, one of us can just keep a "requesting changes" on this to avoid merging "by accident".

For the time being, I've converted this to a Draft PR to avoid accidental merge.

I think the conversation about how to handle new modules once we've completed the transition to standalone plugins #656 is worth it's own conversation. I'll open a new issue for us to discuss. I am also going to try to spend some time seeing how difficult it would be to post standalone releases to this repo, since I think that would be useful regardless of whichever future strategy we choose.

@joemcgill joemcgill mentioned this pull request Dec 19, 2023
3 tasks
@joemcgill
Copy link
Member Author

@felixarntz I forgot that the branch protections on feature branches would block me from continuing to update this branch directly, so I've opened #913 to address the code review feedback and to add initial unit tests.

@joemcgill
Copy link
Member Author

@felixarntz I've opened a new PR against this feature branch that I believe prepares this to be submitted to the the plugin repo for review. I've build the plugin locally by modifying the plugins.json file and have it successfully running on my personal site.

I'm happy to submit this to the Plugin Team for review, but want to make sure that the repo they create will be accessible using the same credentials as the other plugins built from this repo and would appreciate your guidance.

@mukeshpanchal27
Copy link
Member

@joemcgill @swissspidy, this PR is ready for merge. The test suite will be handled in #972. WDYT?

Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to get the ball rolling 👍

@joemcgill joemcgill mentioned this pull request Feb 7, 2024
3 tasks
Copy link
Member Author

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to have you merge this into the feature/modules-to-plugins branch ahead of the changes being proposed in #972, I think I'm just a bit confused about the sequencing of these changes, but I'm sure it's just me being dense.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve again for merge.

@joemcgill joemcgill merged commit 36fe0dc into feature/modules-to-plugins Feb 19, 2024
38 checks passed
@felixarntz felixarntz deleted the feature/auto-sizes branch February 27, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no milestone PRs that do not have a defined milestone for release [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module Proposal: auto-sizes for lazy-loaded images
5 participants