-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove unnecessary blocking style requests #237
Conversation
01c8d3d
to
2167efc
Compare
@@ -39,6 +40,8 @@ | |||
add_filter( 'grunion_should_send_email', '__return_false' ); | |||
// Enable auto-fill using user information. | |||
add_filter( 'jetpack_auto_fill_logged_in_user', '__return_true' ); | |||
// Remove Jetpack CSS on frontend | |||
add_filter( 'jetpack_implode_frontend_css', '__return_false', 99 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find. Looping in @pkevan since he has more experience with JetPack than I do. The front-end and form still appear to work as expected. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the Jetpack docs describe the intention of this filter quite differently, but I've found multiple sources using it for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is something like this found, it's usually best to highlight it with Jetpack themselves, as it could be an opportunity on their side to optimise or a bug in the implementation itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
highlight it with Jetpack themselves
Good call, I'll raise this and see if there's a better way to remove this CSS too, as neither dequeue
or deregister
work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work as advertised and improved page load. I pinged in the comment but I don't think we need to wait on him for a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This goes with the same results as yours on my side.
See #233
The current lighthouse performance score is consistently < 80.
In my sandbox it's between 80 and 90 before these changes. The biggest contributing factor is the LCP metric, and a major factor in that is render blocking styles for dashicons and jetpack:
We don't need either of these on the frontend as far as I can see, so this PR fixes that opportunity by removing them. This results in a score > 90, due to an improved LCP metric:
The initial server response time is bad for me accessing my sandbox in NZ, and is considerably better in prod, so I think we can work on these resource loading optimisations first, and look at server response next if necessary.
Testing
Best to use a sandbox to be as close as possible to prod, and an incognito tab so that no extensions can interfere with results. Run a Lighthouse performance test with and without these changes and compare.
Check that Jetpack and the admin in general still have their required CSS loaded.