Skip to content

Conversation

@ShyamGadde
Copy link
Contributor

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.

…and other groups are empty

Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
@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

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 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

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

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
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!

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.

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

Labels

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

Projects

None yet

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