-
Notifications
You must be signed in to change notification settings - Fork 99
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
Preloading images and stylesheets: <link> vs Early Hints response header #617
base: trunk
Are you sure you want to change the base?
Conversation
…Early Hints header.
…g vs Early Hints header.
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.
Thanks @felixarntz Left some feedback.
if ( null !== $ver ) { | ||
if ( false === $ver ) { | ||
$ver = wp_styles()->default_version; | ||
} | ||
$preload = add_query_arg( 'ver', $ver, $preload ); | ||
} |
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.
When the version of the stylesheet is not define then we didn't get theme version. ex. wp_enqueue_style( 'style', get_stylesheet_uri(), array(), '' );
* @since n.e.x.t | ||
*/ | ||
function perflab_tseh_reset_stylesheet_check() { | ||
delete_option( 'perflab_preload_stylesheet' ); |
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.
Please make sure to delete the new option in uninstall.php
.
// Bail if not a frontend request. | ||
if ( is_admin() || defined( 'XMLRPC_REQUEST' ) || defined( 'REST_REQUEST' ) || defined( 'MS_FILES_REQUEST' ) ) { | ||
return; | ||
} |
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.
Similar conditions are checked in perflab_tseh_send_early_hints_header()
. Add a new function and use it.
} | ||
); | ||
} | ||
add_action( 'plugins_loaded', 'perflab_hieh_send_early_hints_header' ); |
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.
Do we really need all this logic if we can just check the current post thumbnail ID and preload that image if it is available? If there is no thumbnail for the post, then I think we should preload nothing because the first image in the content is not always the hero image. Furthermore, the first image in the content can be far below the fold and in this case preloading it is inefficient. 🤔
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.
@eugene-manuilov What you're saying about the first content image can certainly happen, though at the same time it even applies to the featured image, it entirely depends on the theme where that is displayed. It usually is in the header area but not always.
Each of these concerns already apply to the fetchpriority="high"
and loading="lazy"
features too to be fair.
@mukeshpanchal27 @eugene-manuilov Thank you for the feedback. Just to highlight, this PR is not meant to be merged. It's fine to review it, but there's a couple things in here that I just quickly hacked together and that would certainly not be a proper approach if we wanted to truly make this a module. Just as some context; I'll probably eventually close this, not really worth too much polishing (unless something is broken that even affects the lab tests we're running with this code). |
Summary
This is an experiment. Do not merge.
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.