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 tests for Optimize Embeds #979

Merged

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Feb 9, 2024

Summary

Adds some tests for Optimize Embeds; also complete rename. Using Optimize Embeds because this could broaden scope beyond just "lazy".

Follow up to https://github.com/WordPress/performance/pull/886/files

Checklist

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

Copy link

github-actions bot commented Feb 9, 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: westonruter <westonruter@git.wordpress.org>

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

@adamsilverstein adamsilverstein added [Type] Enhancement A suggestion for improvement of an existing feature [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 labels Feb 9, 2024
module-i18n.php Outdated Show resolved Hide resolved
// Twitter embed.
array(
'<blockquote class="twitter-tweet" data-width="500" data-dnt="true"><p lang="en" dir="ltr"Feedback <a href="https://t.co/anGk6gWkbc">https://t.co/anGk6gWkbc</a></p>&mdash; <a href="https://twitter.com/ChromiumDev/status/1636796541368139777?ref_src=twsrc%5Etfw">March 17, 2023</a></blockquote><script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>',
'<blockquote class="twitter-tweet" data-width="500" data-dnt="true"><p lang="en" dir="ltr"Feedback <a href="https://t.co/anGk6gWkbc">https://t.co/anGk6gWkbc</a></p>&mdash; <a href="https://twitter.com/ChromiumDev/status/1636796541368139777?ref_src=twsrc%5Etfw">March 17, 2023</a></blockquote><script data-lazy-embed-src="https://platform.twitter.com/widgets.js" async charset="utf-8"></script>',
Copy link
Member

Choose a reason for hiding this comment

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

I just had a thought. What if instead of converting script[src] to script[data-lazy-embed-src], it instead set the type attribute of text/x.lazy-loaded-javascript? Clearly out of scope for this PR, but just thought of this alternative. It probably doesn't make any difference, oh except actually wouldn't it be better in that it would prevent the browser from treating this script as an inline script? While the browser hopefully is smart enough to skip parsing an inline script that has no contents, in the worst case it could. That would be bad because inline scripts are blocking. If the src is left-as is and the type is used to mark such lazy-loaded scripts instead, this would be avoided. This is also what Partytown does to mark its scripts via text/partytown.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting! I haven't seen this approach but it makes sense that it would work. feel free to propose on another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Here you go! #983

@westonruter westonruter merged commit 6a08643 into WordPress:feature/lazy-embeds Feb 12, 2024
26 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] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants