-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: generate dynamic docs header links #2055
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.
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
base: gh-pages
Are you sure you want to change the base?
feat: generate dynamic docs header links #2055
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Shubham Oulkar <[email protected]>
Signed-off-by: Shubham Oulkar <[email protected]>
🚦 Lighthouse Results (Mobile & Desktop)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I thought we already had these, or had them in a PR, but I can't find any evidence for that.
Great idea.
Do note though, this will add these anchor links to h2, h3
on all pages of the site!
I didn't check all pages to see if it is messed up anywhere, but I caught some bugs on the docs page at least.
Is there a reason why this is living inside the copycode.js
script file? it could move to the app.js
file inside the jquery DOMContentLoaded callback at the top of the file
}); | ||
|
||
// add heading links | ||
for (const heading of headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const heading of headers) { | |
for (const heading of headers) { | |
if (h.querySelector('.heading-link')) continue; |
Nit: A little paranoid but this will guard against the script running twice somehow by accident or hot reloading or something, preventing us from adding the elements twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want thinking of implementing lazy attach links using IntersectionObserver
or MutationObserver
. WDYT?
Is there a reason why this is living inside the copycode.js script file?
Copy code and header link share same logic and needs asynchronous implementation.
Co-authored-by: Jon Church <[email protected]>
Co-authored-by: Jon Church <[email protected]>
Co-authored-by: Jon Church <[email protected]>
I have addressed all reviews. Please take a look.
jQuery is migrated to web APIs
Yes, this will add anchor on all pages. Here are some issues that are affecting this PR #2055 (comment) and #2056. First is fixed here itself, #2056 needs to be fixed. |
New design
❌ design ❌
