Skip visiting tags in the Admin Bar when optimizing a page#1816
Skip visiting tags in the Admin Bar when optimizing a page#1816westonruter merged 5 commits intotrunkfrom
Conversation
westonruter
added
[Type] Enhancement
westonruter
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1816 +/- ##
==========================================
+ Coverage 65.42% 65.47% +0.05%
==========================================
Files 85 85
Lines 6738 6748 +10
==========================================
+ Hits 4408 4418 +10
Misses 2330 2330
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@westonruter This looks good functionally, just one suggestion on where it's implemented.
| // Elements in the Admin Bar are not relevant for optimization, so skip passing them to tag visitors. | ||
| if ( $processor->is_admin_bar() ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Instead of calling this here, could this be baked into the tag processor class itself? For instance, as long as we're in the admin bar, the tag processor could move on to the next element.
I think that would be better for ergonomics, as it would handle the issue in one place and would avoid having to introduce a public method for something as specific as this.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@westonruter LGTM, though there is a merge conflict now, could you address it?
2 participants
Summary
Fixes #1806
Relevant technical choices
A new private
OD_HTML_Tag_Processor::is_admin_bar()is introduced which returns whether the processor is currently at or inside the Admin Bar. This is used in the::next_tag()method to ensure that it never lands on a tag that is in the Admin Bar.