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 oEmbed optimizer module #886

Merged

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Nov 22, 2023

Summary

Full issue - #888

Attempts to optimize oEmbeds using two techniques:

  • For oEmbeds delivered as iframes, ensure the loading=lazy attribute is present
  • For oEmbeds built using script tags:
    • move the script src to a data attribute
    • add a page level interaction observer that resets src before the user scrolls to the oEmbed

Related: https://core.trac.wordpress.org/ticket/58773

Relevant technical choices

Checklist

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

@adamsilverstein adamsilverstein added [Focus] JS & CSS Issues related to the JS & CSS focus area (formerly JavaScript) [Type] (Legacy) Module Proposal A new module proposal (legacy) labels Nov 22, 2023
@adamsilverstein adamsilverstein marked this pull request as ready for review November 22, 2023 21:26
Copy link
Contributor

@sgomes sgomes left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @adamsilverstein! I think this has the potential for some great wins! 👍

I left some non-blocking comments for your consideration. I'm not sure if I'll have the opportunity to re-review because of some upcoming AFK, so please feel free to move ahead at your discretion!

modules/js-and-css/optimize-oembeds/hooks.php Outdated Show resolved Hide resolved
modules/js-and-css/optimize-oembeds/hooks.php Outdated Show resolved Hide resolved
adamsilverstein and others added 3 commits November 29, 2023 09:09
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein adamsilverstein added the no milestone PRs that do not have a defined milestone for release label Dec 20, 2023
adamsilverstein and others added 7 commits January 24, 2024 07:33
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein
Copy link
Member Author

Thanks for all the feedback and suggestions @westonruter and @mukeshpanchal27! I'm going to work on adding tests.

@westonruter
Copy link
Member

westonruter commented Feb 5, 2024

Two things:

  1. Shouldn't tests be added prior to merging?
  2. No new modules have been merged in a while as they're waiting for migration to standalone plugins. I don't think we're supposed to be adding new modules.

Perhaps what should be done is to create a new feature/lazy-embeds branch off of trunk and then re-base this PR to target it. Then it can be merged prior to tests and we can also wait until we have the module-to-plugins plumbing finished.

Copy link

github-actions bot commented Feb 5, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: sgomes <sergiomdgomes@git.wordpress.org>
Co-authored-by: dmsnell <dmsnell@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein
Copy link
Member Author

Perhaps what should be done is to create a new feature/lazy-embeds branch off of trunk and then re-base this PR to target it. Then it can be merged prior to tests and we can also wait until we have the module-to-plugins plumbing finished.

That sounds good to me!

@westonruter
Copy link
Member

I created the feature/lazy-embeds branch but I can't change the base branch in your PR since it is on a fork. You'll have to do that. Once you've done it we can merge and then proceed with tests.

@adamsilverstein adamsilverstein changed the base branch from trunk to feature/lazy-embeds February 8, 2024 16:46
@adamsilverstein
Copy link
Member Author

I created the feature/lazy-embeds branch but I can't change the base branch in your PR since it is on a fork. You'll have to do that. Once you've done it we can merge and then proceed with tests.

Thanks @westonruter - changed base branch

@westonruter westonruter merged commit f72a7ed into WordPress:feature/lazy-embeds Feb 8, 2024
30 checks passed
@westonruter westonruter added the [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] JS & CSS Issues related to the JS & CSS focus area (formerly JavaScript) no milestone PRs that do not have a defined milestone for release [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin [Type] (Legacy) Module Proposal A new module proposal (legacy)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants