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

Add/sitehealth enqueued assets #1

Merged
merged 22 commits into from
Mar 17, 2022

Conversation

manuelRod
Copy link

@mehigh mehigh requested a review from pbearne December 10, 2021 12:10
Copy link

@pbearne pbearne left a comment

Choose a reason for hiding this comment

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

The tests look really good well done
Most of the comments are around the invaliding the cache

modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
@manuelRod
Copy link
Author

This is the WP issue: WordPress#25

@manuelRod
Copy link
Author

@pbearne ready for the second code review, I added everything we talked about.

Copy link

@pbearne pbearne left a comment

Choose a reason for hiding this comment

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

I don't like leaving a query string with an action in the URL as it reruns the action on a page reload

modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
Copy link

@ThierryA ThierryA left a comment

Choose a reason for hiding this comment

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

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?

modules/audit-enqueued-assets/load.php Outdated Show resolved Hide resolved
'test' => 'enqueued_css_assets',
);

if ( $enqueued_styles > 10 ) {
Copy link

@ThierryA ThierryA Dec 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@ThierryA I've opened the PR in WP WordPress#54 we can follow up in there.

Copy link

@ThierryA ThierryA Dec 17, 2021

Choose a reason for hiding this comment

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

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 ) {

Choose a reason for hiding this comment

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

Same comment as for the stylesheet, we need to grab and sum the total size of assets loaded.

@pbearne pbearne merged commit 306bbec into xwp:trunk Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants