Feature-16-sidebar-eager #65
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
There was a problem hiding this comment.
I like this approach 👍
- We have a standalone fragment block that is reusable (and we will most likely need it again for other use-cases in the project)
- There is no duplication in the sidebar code or hardcodings
- Lighthouse Score is still at 100 ✅
- The eager loading block mechanism can also be used for other usecases if we need it later.
A couple mentions above to make things a little bit more robust and we are ready to merge.
| const fragmentSection = fragment.querySelector(':scope .section'); | ||
| if (fragmentSection) { | ||
| block.closest('.section').classList.add(...fragmentSection.classList); | ||
| block.closest('.fragment-wrapper').replaceWith(...fragmentSection.childNodes); |
There was a problem hiding this comment.
we need to transport the fragmentSection's data set to the target section too, to ensure that additional section metadata configs are added, but without overriding the main's section data set (e.g. data-section-status);
I'm thinking something like:
const mainSection = block.closest('.section');
mainSection.classList.add(...fragmentSection.classList);
mainSection.dataset = Object.assign({}, fragmentSection.dataset, mainSection.dataset);| const targetDivs = divs.filter( | ||
| (d) => d.textContent.trim().toLowerCase() === 'style' || d.textContent.trim().toLowerCase() === 'sidebar', | ||
| ); | ||
| if (targetDivs.length !== 2) { |
There was a problem hiding this comment.
This condition can unfortunately be broken. Take the following example which will say that there is already a sidebar and doesn't need to be autoblocked:
<div class="section-metadata">
<div>
<div>Style</div>
<div>Test1</div>
</div>
</div>
<div class="section-metadata">
<div>
<div>Test2</div>
<div>Sidebar</div>
</div>
</div>Also the Style property can have multiple values coma separated and with spaces, e.g.:
<div class="section-metadata">
<div>
<div>Style</div>
<div>Sidebar, Test1, Test2</div>
</div>
</div>https://github.com/hlxsites/servicenow/blob/main/scripts/aem.js#L453
So we need a condition that is a little bit more robust.
scripts/scripts.js
Outdated
| @@ -189,8 +215,22 @@ function buildAutoBlocks(main) { | |||
| } | |||
| } | |||
|
|
|||
| function detectSidebar(main) { | |||
| export function addH3Spans(elem) { | |||
There was a problem hiding this comment.
This function is not called
andreituicu
requested review from
benpeter,
sunidhiazad,
sergiucoman,
Dereje24,
sourav622,
samittal-SN and
VenkatKrishnaSN
There was a problem hiding this comment.
as an exception for the usual PR review process, I'm approving and merging this PR in state that it is now, as I need the functionality for a demo today.
@florentin please create a new PR for the 2 items discussed above. I cleaned up the addH3Spans function as it was trivial.
Sidebar with eager fragment loading
Partial for #16
US blog
Before: https://main--servicenow--hlxsites.hlx.live/
After: https://feature-16-sidebar-eager--servicenow--hlxsites.hlx.live/blogs/drafts/sardan/maximize-efficiency-hyperautomation
UK blog
Before: https://main--servicenow--hlxsites.hlx.live/
After: https://feature-16-sidebar-eager--servicenow--hlxsites.hlx.live/uk/blogs/drafts/sardan/maximize-efficiency-hyperautomation
UK blog
Before: https://main--servicenow--hlxsites.hlx.live/
After: https://feature-16-sidebar-eager--servicenow--hlxsites.hlx.live/blogs/2023/demo/maximize-efficiency-hyperautomation