Skip to content
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

Introduce OD_Element class #1585

Draft
wants to merge 2 commits into
base: add/embed-optimizer-min-height-reservation
Choose a base branch
from

Conversation

westonruter
Copy link
Member

Important

This is a follow up to #1373. Do not merge this before that is merged into trunk.

See https://github.com/WordPress/performance/pull/1373/files#r1792652840:

See note below on the get_all_denormalized_elements() method that I'd like to instead have $element be an OD_Element class instance which has a $url_metric property to access the OD_URL_Metric instance if is a part of. In the same way, the OD_URL_Metric class can have a $group property to indicate the OD_URL_Metric_Group it is a part of. This would eliminate the need to pass back an array of tuples and instead. So instead, to get the $group this code could do $element->url_metric->group.

@@ -406,10 +414,15 @@ public function get_timestamp(): float {
/**
* Gets elements.
*
* @return ElementData[] Elements.
* @return OD_Element[] Elements.
*/
public function get_elements(): array {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that $url_metric->get( 'elements' ) returns an array of arrays (the underlying values), whereas $url_metric->get_elements() returns the data wrapped in OD_Element objects.

*/
public function get_all_denormalized_elements(): array {
public function get_all_elements(): array {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really like the get_all_elements name. It's more like get_elements_grouped_by_xpath.

* @param mixed $offset Key.
* @return mixed Can return all value types.
*/
public function offsetGet( $offset ) { // phpcs:ignore SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint
Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented ArrayAccess here for back-compat with existing code accessing the offsets. But for the way we access data, perhaps we should have accessor get methods similar to what is done for OD_URL_Metric. So this would have:

  • is_lcp()
  • is_lcp_candidate()
  • get_xpath()
  • get_intersection_ratio()
  • get_intersection_rect()
  • get_bounding_client_rect()

As well as a get() method which would return arbitrary values, especially those which are from an extension-extended schema, like $element->get( 'resizedBoundingClientRect ) in Embed Optimizer.

@westonruter westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label Oct 9, 2024
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.

1 participant