Optimized Page Feed Block #14
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 see some errors on the page, likely because you are not including the GNav on the Artist Hub pages, do we need to add logic to skip adding the GNav on AH pages?
Additionally, the change region modal in the footer does not work and returns
GET https://pagefeed-optimize--adobestock--wbstry.hlx.page/fragments/regions.plain.html 404 (Not Found)
but this isn't likely an issue caused by this PR, just wanted to bring it up.
The CSS/JS changes I can't fully review as there are over 1000 lines in this PR and most of it, as I understand, is a migration from your existing codebase.
| replaceKey, | ||
| } from '../../scripts/scripts/scripts.js'; | ||
|
|
||
| function turnH6intoDetailM(scope) { |
There was a problem hiding this comment.
This file needs a header and JSDoc for all these functions, I would like to see documentation on how the page feed is to be used, by example.
There was a problem hiding this comment.
Sure! I think there's some debate between some a.com projects whether we should spend the bytes to write design contracts or not. I personally lean towards yes but for this file I didn't add them in. I'll do a round on it. 😄
https://page-feed-optimize--stock--wbstry.hlx.page/drafts/solene/page-feed is where you'd be able to find most documented use case for page feed block. There is the json fetch version that we retired so they aren't getting brought over.
There's also https://pagefeed-optimize--adobestock--wbstry.hlx.page/pages/artisthub/community/stock-mentors. Sorry for not keeping all the use cases in one file. The requirement for this specific use case came way after the initial documentation creation and unfortunately fell through.
There was a problem hiding this comment.
Could we add a comment to include these 2 use cases in this file or a README? That way it will be much easier for someone in the future to understand the design and usage examples.
There was a problem hiding this comment.
Sure. I'm actually going to leave the link in PR description as a reference in code and update that page to contain all use cases later. I think it's probably not the best to be using a link from the old repo as documentation. Would that be OK?
franklin/scripts/scripts/scripts.js
Outdated
| @@ -45,6 +45,8 @@ const CONFIG = { | |||
| */ | |||
|
|
|||
| const miloLibs = setLibs(LIBS); | |||
| export const { createTag, getConfig } = await import(`${miloLibs}/utils/utils.js`); | |||
There was a problem hiding this comment.
Bringing in the entire utils library is expensive, do we need to bring everything in? That's why we just lifted createTag and added it to the repo, which can be done with getConfig as well.
There was a problem hiding this comment.
No problem! I'll extract them.
There was a problem hiding this comment.
I took another look at the scripts.js. The self-invoking loadPage() brings the entire utils library as well. Since the browser caches it, all in all we would always have fetched once. I'll move these lines lower in the file to pay the DNS tax after the style loads and consolidate the imports but I don't think this import comes with extra cost.
There was a problem hiding this comment.
Oh good catch, didn't notice that from the initial Milo implementation. In that case, I agree, since it's already cached and our Core Web Vitals aren't impacted much, it should be fine to use the functions from the cloud-hosted milo lib.
|
|
||
| const href = (typeof (a) === 'string') ? a : a.href; | ||
| const path = makeRelative(href); | ||
| if (!path.startsWith('/')) return null; |
There was a problem hiding this comment.
Is this to prevent arbitrary fetches? Did you ensure code cannot be escaped to run arbitrary fetches in the client?
There was a problem hiding this comment.
This is also a migrated function. It was in the utils.js in the old repo and was shared by multiple blocks. I extracted to have it only work for this block.
Just reading the code, I believe what it's trying to do is to do is to only make the attempt to fetch and load the corresponding page-feed-card when the path is an internal link.
So the escape logic is:
- if a link is not an artisthub link, don't fetch and load the page-feed-card
- if a link is an artisthub link, but fetch didn't return a page-feed-card, throw a false-y value, and at card render, the card index is logged in 'trash can' and pagination is skipped.
This is actually one of the reasons why the page-feed block remains valuable in the migration -- it's versatile enough to support links to any source of content. Be that an external link, a pdf file or a modal link.
That said, the makeRelative function naming is surely a little confusing. I'll change that.
Thanks for bringing up the errors. We'll take another look and fix in a different PR. |
|
Includes: required social icons dependent page feed card block loading animation gif file small scripts.js refactor
qiyundai
force-pushed
the
pagefeed-optimize
branch
from
f97bd40 to
1e25173
Compare
|
We can safely ignore the error with gnav. That's solely a draft page issue as it's not in the same content root as the actual project |
|
|
😄 Surprised to see this still open and conflict-free after 5 months. What's the status? Do we still need this? @paradoxministry |
|
@qiyundai I would check with the PM for Artist Hub, I'm not working with that project or team so I don't know the status. |
In this PR, we are trying to bring in a fully upgraded page-feed block that leverages Promise API to load content cards without sacrificing page performance. The authoring side change has been done and QA'd to satisfy the block's requirement once the block is merged.
We are aware some video links will need to be updated. It's only a few. We would advise to upload these videos to adobe.tv and update the links manually.
Test URLs:
Before: https://main--adobestock--adobecom.hlx.page/drafts/qiyun/page-feed-pr?martech=off
After: https://pagefeed-optimize--adobestock--wbstry.hlx.page/drafts/qiyun/page-feed-pr?martech=off