hlxsites / accenture-newsroom

Accenture Newsroom on Adobe Franklin
Apache License 2.0
0 stars 13 forks source link

Refactor: Get fragment description through fetching of plain html #498

Closed jjjaspher closed 9 months ago

jjjaspher commented 10 months ago

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix https://github.com/hlxsites/accenture-newsroom/issues/480

Test URLs:

aem-code-sync[bot] commented 10 months ago

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.

Commits * [e9b7c3c](https://github.com/hlxsites/accenture-newsroom/pull/498/commits/e9b7c3c15c0ee556cfa6f059ed7caaec0b847d0c) :white_check_mark: (latest) * [459bc59](https://github.com/hlxsites/accenture-newsroom/pull/498/commits/459bc5905292ab2e7231ba2230d79026184dc2ce) :white_check_mark: * [fb04e86](https://github.com/hlxsites/accenture-newsroom/pull/498/commits/fb04e86e8de7556b3a87f67ec178d74044930e62) :white_check_mark: * [5b6a52d](https://github.com/hlxsites/accenture-newsroom/pull/498/commits/5b6a52d2a707fbd015c76db7b3a23e4a4fba397d) :white_check_mark: * [e2e9fc1](https://github.com/hlxsites/accenture-newsroom/pull/498/commits/e2e9fc15d1023f762a0e4e12ab3ec9359dea4188) :white_check_mark:
aem-code-sync[bot] commented 10 months ago
Page Scores Audits
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS
shsteimer commented 10 months ago

@jjjaspher I took a closer look at perf and it looks to me to be CLS. I think if we can add a min-height on .newslist-item that should remove most if not all of the cls (that height will need to be a bit of a guess, and probably different across breakpoints).

One option would be to add that min-height only while the description is loading, so something like:

  1. add a class to the item, description-pending or whatever
  2. tie min-height to that class
  3. once it's loaded, remove the class, and thus the min-height, allowing the item's height to just be auto
jjjaspher commented 10 months ago
  • I think long description is used by search. can we change that to just use description instead?

Hi @shsteimer , our description column is not a clean data, it was being cutoff from many articles. image

For the CLS issue, I implemented the min-height for both newslist card and search result. We're now back to 100 performance score. Please let me know if there are other concern. Thanks!

shsteimer commented 10 months ago

@jjjaspher everything looks good and is good to merge IMHO.

I do think we still need to do something about search as that uses the long description from the query index at https://github.com/hlxsites/accenture-newsroom/blob/main/blocks/newslist/newslist.js#L112

but can probably do that in a separate PR