-
Notifications
You must be signed in to change notification settings - Fork 224
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
Rewrite google fonts with SaaS visit #7162
Rewrite google fonts with SaaS visit #7162
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
'do_not_optimize' => false, | ||
'bypass' => false, |
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.
Why do we remove those? I don't believe the optimization should be applied when using the bypass or donotoptimize
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.
Sorry @remyperona didn't notice ur comment, we need to remove those to work with SaaS visits because SaaS visits the page with those query strings:
/?nowprocket=1&no_optimize=1&wpr_imagedimensions=1
And we call this method rewite_fonts
with rocket_performance_hints_buffer
which is fired only when wpr_imagedimensions
query string is there.
Do u see any issue when using that with rocket_buffer?
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.
Not having those checks is going to apply the optimization when using the nowprocket
parameter or the DONOTROCKETOPTIMIZE
constant
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.
Ok, I have an idea to fix that, let me test it
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.
Now I separated context for normal optimizations from SaaS visits optimizations.
@wordpressfan Thanks for the PR.
|
checking those points now |
@Mai-Saad for the first point, as we discussed in a call, that's happening because of preload is enabled so it visits the pages and load google fonts locally. Checking now the second point. |
…with-saas-visit # Conflicts: # inc/Engine/Media/Fonts/Frontend/Controller.php
@Mai-Saad I tested the second point after finishing the review comment from remy and I can see it's working properly, I used the following snippet to insert the font into homepage and insert HTML div that uses that font:
I'm fixing tests now to send it to review but u can validate from ur side to get some time while it's being reviewed. |
@wordpressfan Thanks for the update. Kindly note that using local fonts with used CSS is working when lcp or lrc is enabled with RUCSS but not working when lcp & lrc are disabled |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Working fine now |
Description
Fixes #7070
Here we make sure that we host google fonts when SaaS visits the frontend page with features:
Type of change
Detailed scenario
With SaaS visits, we serve google fonts locally if the option is enabled.
Technical description
Documentation
Everything is mentioned in the issue itself and grooming.
New dependencies
No
Risks
No
Mandatory Checklist
Code validation
Code style
Unticked items justification
Additional Checks