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.

Sign up for GitHub

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

Jump to bottom

chore: try section loader #434

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: try section loader #434

wants to merge 4 commits into from

Conversation

davidnuescheler
Copy link
Contributor

@davidnuescheler davidnuescheler commented Dec 9, 2023 •
edited by kptdobe
Loading

@aem-code-sync AEM Code Sync
Copy link

aem-code-sync bot commented Dec 9, 2023 •
edited
Loading

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@aem-code-sync AEM Code Sync
Copy link

aem-code-sync bot commented Dec 9, 2023 •
edited
Loading

Page Scores Audits Google
/developer/tutorial PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/docs/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/home PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@davidnuescheler
Copy link
Contributor Author

image

ramboz
ramboz approved these changes Dec 12, 2023
View reviewed changes
Copy link
Collaborator

@ramboz ramboz left a comment

Choose a reason for hiding this comment

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

I like the overall simplification.

🤞🏼 Hoping that the SI improvements are not just an edge case of the page's content and can be repeated on other sites as well

scripts/scripts.js Outdated
@@ -598,7 +597,8 @@ async function loadEager(doc) {
decorateBreadcrumb(main);
prepareSideNav(main);
document.body.classList.add('appear');
await waitForLCP(LCP_BLOCKS);
const firstSection = main.querySelector('.section');
await loadSection(firstSection, waitForImage);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I sometimes wonder if this shouldn't be even implementation details of the responsible block that should await internally before the end of the decoration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would put extra burden on every block that could possibly host the LCP and worry about the distinction of being hosting the LCP and not...

scripts/lib-franklin.js Outdated Show resolved Hide resolved
@aem-code-sync AEM Code Sync
Copy link

aem-code-sync bot commented Dec 14, 2023 •
edited
Loading

Page Scores Audits Google
/developer/tutorial PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/docs/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/home PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@davidnuescheler
Copy link
Contributor Author

image

@davidnuescheler
Copy link
Contributor Author

also, there was some very weird CLS suppression in the sidenav that set the main content to display:none which might have added to the SI looking at the screenshots.

image image

@kptdobe
Copy link
Contributor

kptdobe commented Dec 15, 2023 •
edited
Loading

I could produce a huge CLS locally once: my performance tool shows me this:

image

Actually, I can reproduce: if you scroll, the first video will load, this is what triggers the CLS above. Probably not the "thing".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rofe rofe rofe approved these changes

@ramboz ramboz ramboz approved these changes

@fkakatie fkakatie Awaiting requested review from fkakatie

@kptdobe kptdobe Awaiting requested review from kptdobe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidnuescheler @kptdobe @ramboz @rofe