-
Notifications
You must be signed in to change notification settings - Fork 227
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
Depend on php to identify elements and depth #6929
Depend on php to identify elements and depth #6929
Conversation
…-elements-and-depth
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 |
That's a good idea @jeawhanlee |
…/depend-on-php-to-identify-elements-and-depth # Conflicts: # inc/Engine/Optimization/LazyRenderContent/Frontend/Controller.php
By default set depth is 2, and actually, it's applied up to 3rd child after body.
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.
Working as expected. (includes validation for #6904)
- we can have enhancement later https://wp-media.slack.com/archives/CUKB44GNN/p1725008472171879?thread_ts=1724992642.140039&cid=CUKB44GNN
- if nothing is needed for this now https://wp-media.slack.com/archives/CUKB44GNN/p1725009496716359 then we can merge
@MathieuLamiot we will need to revert back to calculating the distance relative to the viewport.
What @Mai-Saad reported here will be complicated to handle as we can't restrict customers to a specific value and it will require a lot of customer education to not cost support time and efforts - https://wp-media.slack.com/archives/CUKB44GNN/p1725009496716359 Sorry for the oversight. |
@DahmaniAdame to avoid further confusion, I'd prefer we have a dedicated GH issue: not only JS must be changed but also the default threshold value (PHP side). Note that this means results will be dependent on the screensize of the person visiting the page. So someone with a small screen might pickup some LRC elements that will be lazy rendered while being in the viewport of another user with taller screen. I am not sure this is the best solution as results won't be always reproducible. |
Got it. I will create a separate GH issue.
That was the thinking behind it. But it will cause more trouble and confusion for customers. The relative distance to the viewport is still usable though and should work fine with the constraint that we have now. Having a couple of elements toggling |
Description
This PR is a reflect of the changes of that PR: wp-media/rocket-scripts#26
Explain how this code impacts users.
It'll not impact users, that's an enhancement.
Type of change
Detailed scenario
Everything is mentioned in the issue itself and grooming discussion.
Technical description
Documentation
We will depend on php to identify elements and depth.
New dependencies
No
Risks
No
Mandatory Checklist
Code validation
Code style
Additional Checks