-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
tools: add button to copy code example to clipboard #46928
tools: add button to copy code example to clipboard #46928
Conversation
0e458bf
to
403718c
Compare
cc @nodejs/website |
@ovflowd Since doc rendering might be changed to a different mechanism, is there any chance this might be better implemented elsewhere? |
The generation will be done elsewhere in the future, and the tooling will be an NPM package. I believe adding a "Copy code" feature is OK for now. So as you know, this code might be discarded entirely in the future. |
Thank you for triaging @debadree25 @tniessen Hi @ovflowd, I agree that the Thanks for letting me know earlier, so I would not plan to spend too much time on the legacy code 😄 |
I never said anything about nodejs.dev; from where did you assume that? 😅 I have no idea what you mean by this going to nodejs.dev -- edit I think I got what you're referring to; maybe you saw the "test/playground" we're doing on https://nodejs.dev/api/; just to let you know, that's not what I meant with the new docs tooling. There's an issue on nodejs/next-10#166 that is what I was referring to. |
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.
Knowing this is mostly temporary code, I'm okay with the current logic. There are ways of course of optimising this, but let's not be picky for now.
My suggestion would to expose a callback that the button itself can use instead of registering who knows how many event listeners, but that's fine, IMHO.
LGTM
@ovflowd after reading your previous reply, then on the side bar github explore/repositories suggest it nodejs/nodejs.dev to me 😅 I had a look and assumed that would be where the future of the nodejs doc would be, apologise that I made a wrong guess ;) Thanks for pointing me to the right direction at nodejs/next-10#166, I shall give a read, cheers mate 👍 |
function setupCopyButton() { | ||
const buttons = document.querySelectorAll('.copy-button'); | ||
buttons.forEach((button) => { | ||
button.addEventListener('click', (el) => { |
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.
The event handler may be able to extract.
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.
Hi @btea thank you for your feedback, I am just not too sure if it is worth the effort of function extraction.
- The code is temp and won't live in the future doc tool
- the function is relatively short and straightforward
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.
Yeah, what you said makes sense. I made this point because each page of the nodejs official website has a lot of content, and some have many code blocks, so there may be many bindings of the same event, so I think it is worth extracting.
Anyway, I think this feature looks great 👍
would you guys mind having a look at this PR again? @debadree25 @tniessen 👀 |
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.
Hey, I think this is LGTM, leaving it open for a little while more before landing in case anyone would like to chime in, would land this tomorrow maybe.
also cc'ing @nodejs/website for good measure
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.
LGTM
I think there is enough approval now, landing this |
Commit Queue failed- Loading data for nodejs/node/pull/46928 ✔ Done loading data for nodejs/node/pull/46928 ----------------------------------- PR info ------------------------------------ Title tools: add button to copy code example to clipboard (#46928) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch jakecastelli:tool-add-button-to-copy-code-to-clipboard -> nodejs:main Labels doc, tools, author ready Commits 6 - tools: add button to copy code example to clipboard - fix: keep setup function naming consistent - fix: improve copy button naming - fix: move copy button to the right - fix: improve button style - fix: add indicator for copied Committers 1 - jakecastelli <959672929@qq.com> PR-URL: https://github.com/nodejs/node/pull/46928 Refs: https://github.com/nodejs/node/issues/46894 Reviewed-By: Debadree Chatterjee ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46928 Refs: https://github.com/nodejs/node/issues/46894 Reviewed-By: Debadree Chatterjee -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 03 Mar 2023 08:41:26 GMT ✔ Approvals: 1 ✔ - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/46928#pullrequestreview-1357171087 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46928 From https://github.com/nodejs/node * branch refs/pull/46928/merge -> FETCH_HEAD ✔ Fetched commits as a64469dcf10a..bb8bb125f177 -------------------------------------------------------------------------------- [main 088bb4b4b0] tools: add button to copy code example to clipboard Author: jakecastelli <959672929@qq.com> Date: Fri Mar 3 18:55:16 2023 +1030 2 files changed, 32 insertions(+), 2 deletions(-) [main 67fa1dfc9f] fix: keep setup function naming consistent Author: jakecastelli <959672929@qq.com> Date: Fri Mar 3 22:51:30 2023 +1030 1 file changed, 2 insertions(+), 2 deletions(-) [main cbfd343ba5] fix: improve copy button naming Author: jakecastelli <959672929@qq.com> Date: Fri Mar 3 22:54:25 2023 +1030 1 file changed, 4 insertions(+), 4 deletions(-) [main 7c8f00dd20] fix: move copy button to the right Author: jakecastelli <959672929@qq.com> Date: Sat Mar 4 22:49:10 2023 +1030 1 file changed, 5 insertions(+) [main 2353ea64f9] fix: improve button style Author: jakecastelli <959672929@qq.com> Date: Sun Mar 5 23:30:41 2023 +1030 1 file changed, 22 insertions(+) [main 4b1a985554] fix: add indicator for copied Author: jakecastelli <959672929@qq.com> Date: Sun Mar 5 23:31:00 2023 +1030 1 file changed, 6 insertions(+) ✔ Patches applied There are 6 commits in the PR. Attempting autorebase. Rebasing (2/12)https://github.com/nodejs/node/actions/runs/4518347599 |
Landed in 94ec71d |
PR-URL: nodejs/node#46928 Refs: nodejs/node#46894 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Fixes: #46894
Open the PR early for some initial feedback.
Quick demo:
https://user-images.githubusercontent.com/38635403/222962223-4a6e21af-4a8b-420d-8d9b-10b8a1138ea6.mov