hlxsites / delta

https://main--delta--hlxsites.hlx.page/us/en/skymiles/overview
Apache License 2.0
2 stars 3 forks source link

feat: implement ToC block #77

Closed askayastha22 closed 1 year ago

askayastha22 commented 1 year ago

Fix #56

Test URLs:

aem-code-sync[bot] commented 1 year ago

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed. In case there are problems, just click the checkbox below to rerun the respective action.

aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
askayastha22 commented 1 year ago

@ramboz After analyzing multiple docs with ToC, I found that relying on custom logic to generate ToC titles dynamically isn't reliable because they aren't specified in a particular order. Therefore, I decided to let the authors add ToC titles (without the link) in the block itself. If they match with the textContent of the headings or paragraph, JS will add the appropriate internal links.

askayastha22 commented 1 year ago

@ramboz @davidnuescheler One thing I haven't addressed in this implementation is that the ToC title can be different than the section textContent.

For instance, if you look at https://www.delta.com/us/en/skymiles/airline-credit-cards/international-credit-cards. "Delta SkyMiles Credit Cards in Asia" links to "Delta SkyMiles Credit Cards in Japan".

Should we give the authors the option to update the ToC title while linking to a different textContent? If yes, then what would the ToC table look like? The table format would need to accommodate the existing logic for generating links as well as let the authors specify a different ToC title.

ramboz commented 1 year ago

@askayastha22 Let's say "no" for now. We can later improve the block if needed and let the customer implement each link manually as an override… but let's handle that separately and only if explicitly requested by the customer

ramboz commented 1 year ago

I found that relying on custom logic to generate ToC titles dynamically isn't reliable because they aren't specified in a particular order

@askayastha22 I would then propose using regular links with word bookmarks as targets. This lets you decouple the ToC entirely from the actual document. It's independent of possible typos and whitespace mismatches.

And if the block content is left empty and you just have a "ToC" heading, then fall back to extracting from the DOM in the order they appear

aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
askayastha22 commented 1 year ago

@ramboz Linter is complaining about this because there was a merge in the main branch yesterday. Is this a typo?

us/en/skymiles/styles/styles.css
 40:23  ✖  Expected "Whitney-Light" to be "whitney-light"  value-keyword-case

1 problem (1 error, 0 warnings)

ramboz commented 1 year ago

You can ignore the linting error, I'll fix that separately

aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
askayastha22 commented 1 year ago

@ramboz Linter seems to be misinterpreting if (!!currentToc) {.

/home/runner/work/delta/delta/us/en/skymiles/blocks/toc/toc.js
Error:   4:7  error  Redundant double negation  no-extra-boolean-cast

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.
askayastha22 commented 1 year ago

The lighthouse score went from 100 to 84 when all I did was fix some indentation. 🤔

ramboz commented 1 year ago

@askayastha22 Yeah… it is indeed redundant, we can skip it if needed. It just enforces a boolean evaluation rather than testing against an object. it's a bit cleaner but won't have any different behavior in the end. For the LHS, it's always a bit flaky. you'd have to run it a few times in a row to make sure it's really degraded

aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI
aem-code-sync[bot] commented 1 year ago
Page Score PSI Audit Google
/drafts/alex/import/us/en/skymiles/airline-credit-cards/american-express-business-cards SI FCP LCP TBT CLS PSI