| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Co-authored-by: Adam Silverstein <adamjs@google.com>
Co-authored-by: Adam Silverstein <adamjs@google.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
@pbearne This is a solid start. I left a few mostly small comments below, but there are two bigger things among them which are critical but missing here:
Sorry, something went wrong.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
|
I verified this works as expected in a classic theme like twentytwelve. when testing with twentytwentythree however, I noticed that the first image is lazy loaded, so something isn't quite working with the logic in core for block themes. I'm going to dig into this a bit further to see why. cc: @felixarntz |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
As we had discussed earlier, this is caused by the block theme "double" pass at the content (which can help performance). To fix this we need to avoid "counting" the first pass by block themes. I noticed in this case the $context is false, so arrived at this simple solution which fixes the issue in my testing: WordPress/wordpress-develop#3538 I will open up a Trac ticket to discuss further. |
Sorry, something went wrong.
There was a problem hiding this comment.
@pbearne @adamsilverstein Overall this looks solid, though there are a few things we need to fix as they can introduce bugs.
In light of the currently ongoing work to fix image lazy-loading in block themes (see https://core.trac.wordpress.org/ticket/56930), we should make sure to test prior to merge that this works as expected in a block theme.
Looking at the code here, things look like this may not be affected by the double execution bug from core (due to the more explicit context check present here), but we need to verify that. Adding fetchpriority="high" to too many images may be even worse than lazy-loading too many images.
Sorry, something went wrong.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
|
@felixarntz Addressed feedback and ready for another review |
Sorry, something went wrong.
I will double check this. |
Sorry, something went wrong.
|
Milestoning this for the 1.8.0 release coming in December, as it's shaping up for merge by then. I'll give it another review later this week. |
Sorry, something went wrong.
There was a problem hiding this comment.
@pbearne @adamsilverstein The implementation here LGTM, with the following caveats / known issues:
Both of these problems are not critical, since at least this PR doesn't make it worse than it is; and both of these problems need to be fixed separately, so this is good to merge IMO.
Will update the module description though, as it's a bit too technical and doesn't describe the performance impact.
Sorry, something went wrong.
|
FYI the PR whatwg/html#8470 is where adding fetchpriority to the HTML spec is being worked on. This will be critical before we can eventually merge it into WordPress core. But for the plugin shipping this already should be fine. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks @pbearne @adamsilverstein LGTM
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Fixes #477
Relevant technical choices
Checklist