feat: generate dynamic docs header links#2055
feat: generate dynamic docs header links#2055ShubhamOulkar wants to merge 15 commits intoexpressjs:gh-pagesfrom
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ShubhamOulkar
changed the title
Signed-off-by: Shubham Oulkar <oulkarshubhu@gmail.com>
Signed-off-by: Shubham Oulkar <oulkarshubhu@gmail.com>
🚦 Lighthouse Results (Mobile & Desktop)
|
jonchurch
left a comment
jonchurch
left a comment
There was a problem hiding this comment.
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
Co-authored-by: Jon Church <me@jonchurch.com>
Co-authored-by: Jon Church <me@jonchurch.com>
Co-authored-by: Jon Church <me@jonchurch.com>
|
I have addressed all reviews. Please take a look.
Yes, I will move this code to app.js. Just a reminder, jQuery is migrated to web APIs. So no more jquery.
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. |
|
bump @jonchurch |
Co-authored-by: Jon Church <me@jonchurch.com>
There was a problem hiding this comment.
Why is this new logic here instead of in app.js?
|
@bjohansebas, I think astro can do it in better way. You can close this PR if not necessary. |
|
astro can do it in better way. |
3 participants
New design