Conversation
…blob/master/site-health-audit-enqueued-assets.php imported as a module and some little refactoring.
pbearne
left a comment
pbearne
left a comment
There was a problem hiding this comment.
The tests look really good well done
Most of the comments are around the invaliding the cache
|
This is the WP issue: WordPress#25 |
|
@pbearne ready for the second code review, I added everything we talked about. |
pbearne
left a comment
pbearne
left a comment
There was a problem hiding this comment.
I don't like leaving a query string with an action in the URL as it reruns the action on a page reload
There was a problem hiding this comment.
Thanks for working on this 😄 I believe this is still WIP although the this PR is not appearing as a "Draft". Nonetheless I left a few early comments without doing a detailed review yet. The approach to trigger an asset warning is quite weak at the moment and need some more work.
By the way, why opening a PR agains this fork rather than on the https://github.com/WordPress/performance repo, that makes it hard for others to follow along with the review. Could we continue the conversation on the main repo please?
| 'test' => 'enqueued_css_assets', | ||
| ); | ||
|
|
||
| if ( $enqueued_styles > 10 ) { |
There was a problem hiding this comment.
This signal needs to be more robust. The number of stylesheets is somehow a week signal, what really matter is the totally size. We need to grab and sum the size of all stylesheets and define a threshold for the warning.
There was a problem hiding this comment.
@ThierryA I've opened the PR in WP WordPress#54 we can follow up in there.
There was a problem hiding this comment.
Thank you @manuelRod, much appreciated. Left the same comments on the new PR to continue the conversation there.
| 'test' => 'enqueued_js_assets', | ||
| ); | ||
|
|
||
| if ( $enqueued_scripts > 10 ) { |
There was a problem hiding this comment.
Same comment as for the stylesheet, we need to grab and sum the total size of assets loaded.
…s and unit test reordering.
- renaming transients - view_site_health_checks capability check to avoid non logged and non view_site_health_checks users to trigger checks - is_front_page check to avoid creating warnings in any random page.
…_url. Changing hooks to capture enqueued scripts/styles and inlines.
3 participants
This was the original plugin to be moved to modules:
https://github.com/audrasjb/site-health-audit-enqueued-assets/blob/master/site-health-audit-enqueued-assets.php