-
Notifications
You must be signed in to change notification settings - Fork 799
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
Various: Use wp_resource_hints #16305
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16305 Scheduled Jetpack release: July 7, 2020. |
eebd46d
to
8f951fc
Compare
The WP.com side diff is not needed; will be handled part of a larger work to get Likes back in sync. |
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.
I wonder if maybe we could take that opportunity to move this to the Assets package. What do you think?
@jeherve I hadn't considered that. Yeah, let's get something else out of the mega class. |
ddb4272
to
f87a198
Compare
f87a198
to
0a44504
Compare
3b88596
to
4079eeb
Compare
4079eeb
to
1b5a0d0
Compare
I moved it to the |
Caution: This PR has changes that must be merged to WordPress.com |
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.
This tests well for me, it should be good to merge. I only have one question, but that's really nothing super important.
'//i1.wp.com', | ||
'//i2.wp.com', | ||
) ); | ||
\Automattic\Jetpack\Assets::add_resource_hint( |
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.
Any specific reason why you did it this way for this module and for VideoPress, vs. calling use
at the top of the file?
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.
Just since it was the only usage in that file and likely to always be the only usage given that the module file doesn't do much.
Closes #10352 ( props @willstocks )
Fixes #4838
Fixes #8090 ( This does not create a filter, but rather utilizes the existing WP filter that can be used for the ends requested in that issue. ).
When
Jetpack::dns_prefetch
was included, WordPress did not have a native solution, which was implemented awhile ago now.Changes proposed in this Pull Request:
wp_resource_hints
instead of rolling out own.Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
<head>
of a front-end page. Ensure the various Jetpack-related domains are present as<link rel="dns-prefetch"...>
Proposed changelog entry for your changes: