티스토리 수익 글 보기

티스토리 수익 글 보기

Add `fetchpriority=high` to `IMG` when it is the LCP element on desktop and mobile with other viewport groups empty by ShyamGadde · Pull Request #1723 · WordPress/performance · GitHub
Skip to content

Add fetchpriority=high to IMG when it is the LCP element on desktop and mobile with other viewport groups empty #1723

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.

Sign up for GitHub

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

Jump to bottom
Merged
westonruter merged 7 commits into from
Dec 9, 2024
Merged

Add fetchpriority=high to IMG when it is the LCP element on desktop and mobile with other viewport groups empty #1723

westonruter merged 7 commits into from
Dec 9, 2024

Conversation

@ShyamGadde
Copy link
Contributor

@ShyamGadde ShyamGadde commented Dec 6, 2024

Summary

Fixes #1710

Relevant technical choices

  • Updated OD_URL_Metric_Group_Collection::get_common_lcp_element() to also consider an LCP element as common if it is identical in the narrowest and widest viewport groups, with all intermediate groups remaining empty.

@ShyamGadde
Consider an LCP element as common if common in first and last groups …
2039994 
…and other groups are empty

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
@ShyamGadde
Add test case for new logic
ac723e1 
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
@ShyamGadde
Add missing since tag
55ac18a 
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
@ShyamGadde ShyamGadde force-pushed the update/allow-high-fetchpriority-if-lcp-on-atleast-mobile-and-desktop branch from 634fd4f to 55ac18a Compare December 6, 2024 16:46
@ShyamGadde ShyamGadde marked this pull request as ready for review December 6, 2024 16:51
@github-actions
Copy link

github-actions bot commented Dec 6, 2024 •
edited
Loading

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: ShyamGadde <shyamgadde@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

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

westonruter
westonruter reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This is looking great! Could you also update \Test_OD_URL_Metric_Group_Collection::test_get_common_lcp_element() to add a data provider to test each of the return conditions? I see right now it is lacking in that it only tests the case where it returns an OD_Element. It isn’t testing the cases when it can return null.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Dec 6, 2024
@ShyamGadde @westonruter
Refactor LCP conflict check to use foreach
25cb61c 
Co-authored-by: Weston Ruter <westonruter@google.com>
felixarntz
felixarntz approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Great work @ShyamGadde, LGTM!

@ShyamGadde
Update test case test_get_common_lcp_element
3540d9e 
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
@ShyamGadde
Set default parameter value for test case helper function
952f04c 
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
@felixarntz felixarntz mentioned this pull request Dec 9, 2024
Merged
2 tasks
westonruter
westonruter reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Test cases look great, although I believe they can be a bit simplified. No need to include OD_Element::class in the data provider since it’s always going to be the same if the expected is not null.

@ShyamGadde @westonruter
Simplify data provider for test_comment_lcp_element
5af021d 
Co-authored-by: Weston Ruter <westonruter@google.com>
westonruter
westonruter approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

It is a gift! 🎁

@westonruter westonruter merged commit d13f33c into WordPress:trunk Dec 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

@felixarntz felixarntz felixarntz approved these changes

Assignees

No one assigned

Labels

[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Milestone

optimization-detective 0.9.0

Development

Successfully merging this pull request may close these issues.

Allow fetchpriority=high to be added to IMG when it is the LCP element on desktop and mobile with other viewport groups empty

3 participants

@ShyamGadde @westonruter @felixarntz