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.

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

Implement full support for intersectionRect/boundingClientRect, fix viewportRect typing, and harden JSON schema #1411

Merged
merged 14 commits into from
Jul 30, 2024
Merged

Implement full support for intersectionRect/boundingClientRect, fix viewportRect typing, and harden JSON schema #1411

merged 14 commits into from
Jul 30, 2024

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 29, 2024 •
edited
Loading

  • Add remaining properties of intersectionRect/clientBoundingRect to the JSON Schema.
  • Fix type of viewport rect which has integer width/height versus DOMRect which has floats. I caught onto this via Query Monitor reporting:
Implicit conversion from float 739.9976196289062 to int loses precision in /var/www/html/wp-content/plugins/optimization-detective/class-od-url-metrics-group-collection.php on line 486
  • Ensure all properties in the JSON Schema are required and that additionalProperties are not allowed.
  • Refactor tests to reuse a get_sample_url_metric() method in the shared helper trait rather than duplicate the logic across various test classes. This is a follow-up to Move Optimization Detective test cases into separate files and add helper trait to reduce code duplication #1405 which refactored the tests for Optimization Detective.
  • Clean up @throws tag usages, removing where obsolete and suppressing with @noinspection where irrelevant.
  • Fix passing the actual function name to the OD_HTML_Tag_Processor::warn() method.

Tip

Suppress whitespace when reviewing the changes in this pull request!

@westonruter
Implement full support for intersectionRect/boundingClientRect, fix v…
f7ed282 
…iewportRect typing, and harden JSON schema

* Add remaining properties of DOMRect to the JSON Schema.
* Fix type of viewport rect which has integer width/height versus DOMRect which has floats.
* Ensure all properties are required and additionalProperties are false.
@westonruter westonruter added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Jul 29, 2024
@westonruter westonruter force-pushed the fix/od-schema branch 2 times, most recently from 2764ea8 to 276dfa9 Compare July 29, 2024 22:57
@westonruter westonruter marked this pull request as ready for review July 29, 2024 22:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 29, 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: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@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 requested a review from swissspidy July 29, 2024 22:59
adamsilverstein
adamsilverstein reviewed Jul 29, 2024
View reviewed changes
plugins/optimization-detective/class-od-url-metric.php Outdated
),
'height' => array(
'type' => 'number',
'minimum' => 0,
'type' => 'number', // An 'unrestricted double'.
Copy link
Member

Choose a reason for hiding this comment

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

why is this comment "an unrestricted double" and below L109 etc its "A double"? Are they different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is what it is referred to in the DOM spec: https://drafts.fxtf.org/geometry/#dom-domrectreadonly-width

This is probably too much information 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 7399131

adamsilverstein
adamsilverstein reviewed Jul 29, 2024
View reviewed changes
plugins/optimization-detective/tests/storage/test-rest-api.php
'intersectionRect' => array(
'width' => 640,
'height' => 480,
'wooHoo' => 'bad',
Copy link
Member

Choose a reason for hiding this comment

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

:)

@adamsilverstein
Copy link
Member

Overall looks good and all tests pass! I left one question.

@westonruter @adamsilverstein
Reduce duplication and redundancy
7399131 
Co-authored-by: Adam Silverstein <adamjs@google.com>
@westonruter westonruter mentioned this pull request Jul 30, 2024
Merged
2 tasks
swissspidy
swissspidy reviewed Jul 30, 2024
View reviewed changes
plugins/auto-sizes/tests/test-optimization-detective.php
@@ -3,6 +3,8 @@
* Tests for auto-sizes plugin's optimization-detective.php.
*
* @package auto-sizes
*
* @noinspection PhpUnhandledExceptionInspection
Copy link
Member

Choose a reason for hiding this comment

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

That's a PHPStorm thing, right? Do we usually have IDE-specific annotations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, elsewhere in the codebase I've been using them.

@westonruter westonruter merged commit e34711d into trunk Jul 30, 2024
17 of 18 checks passed
@westonruter westonruter deleted the fix/od-schema branch July 30, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@felixarntz felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

@joemcgill joemcgill Awaiting requested review from joemcgill joemcgill is a code owner

@mukeshpanchal27 mukeshpanchal27 Awaiting requested review from mukeshpanchal27 mukeshpanchal27 is a code owner

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein adamsilverstein is a code owner

Assignees
No one assigned
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Bug An existing feature is broken
Projects
WP Performance 2024
Status: Done 😃
Milestone
optimization-detective 0.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@westonruter @adamsilverstein @swissspidy