-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add fetchpriority=high
to IMG
when it is the LCP element on desktop and mobile with other viewport groups empty
#1723
Conversation
…and other groups are empty Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
634fd4f
to
55ac18a
Compare
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 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
.
plugins/optimization-detective/class-od-url-metric-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-od-url-metric-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-od-url-metric-group-collection.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Weston Ruter <westonruter@google.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.
Great work @ShyamGadde, LGTM!
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.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.
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
.
plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Weston Ruter <westonruter@google.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.
It is a gift! 🎁
Summary
Fixes #1710
Relevant technical choices
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.