Skip to content
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

Conversation

jakecastelli
Copy link
Member

@jakecastelli jakecastelli commented Mar 3, 2023

Fixes: #46894

Open the PR early for some initial feedback.

  • support js flavor (cjs, mjs)
  • support code snippet without js flavor
  • fix the button style

Quick demo:
https://user-images.githubusercontent.com/38635403/222962223-4a6e21af-4a8b-420d-8d9b-10b8a1138ea6.mov

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Mar 3, 2023
@jakecastelli jakecastelli force-pushed the tool-add-button-to-copy-code-to-clipboard branch from 0e458bf to 403718c Compare March 3, 2023 08:43
@jakecastelli jakecastelli changed the title [WIP] tool: add button to copy code example to clipboard tools: add button to copy code example to clipboard Mar 3, 2023
@jakecastelli
Copy link
Member Author

Screenshot 2023-03-05 at 11 31 15 pm

@jakecastelli jakecastelli marked this pull request as ready for review March 5, 2023 13:07
@debadree25
Copy link
Member

cc @nodejs/website

@tniessen
Copy link
Member

tniessen commented Mar 6, 2023

@ovflowd Since doc rendering might be changed to a different mechanism, is there any chance this might be better implemented elsewhere?

@ovflowd
Copy link
Member

ovflowd commented Mar 6, 2023

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.

@jakecastelli
Copy link
Member Author

Thank you for triaging @debadree25 @tniessen

Hi @ovflowd, I agree that the nodejs.dev would be much easier to work with as this one is really adding feature to the legacy code. I am ok with discarding it after we moved to nodejs.dev, it is just a good addition for the moment.

Thanks for letting me know earlier, so I would not plan to spend too much time on the legacy code 😄

@ovflowd
Copy link
Member

ovflowd commented Mar 7, 2023

Hi @ovflowd, I agree that the nodejs.dev would be much easier to work with as this one is really adding feature to the legacy code. I am ok with discarding it after we moved to nodejs.dev, it is just a good addition for the moment.

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.

Copy link
Member

@ovflowd ovflowd left a 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 :shipit:

@jakecastelli
Copy link
Member Author

@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) => {
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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 👍

@jakecastelli
Copy link
Member Author

would you guys mind having a look at this PR again? @debadree25 @tniessen 👀

Copy link
Member

@debadree25 debadree25 left a 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

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2023
Copy link

@shanpriyan shanpriyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@debadree25
Copy link
Member

I think there is enough approval now, landing this

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 25, 2023
@nodejs-github-bot
Copy link
Collaborator

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 <[email protected]>
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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 Date: Sat Mar 4 22:49:10 2023 +1030
 1 file changed, 5 insertions(+)
[main 2353ea64f9] fix: improve button style
 Author: jakecastelli <[email protected]>
 Date: Sun Mar 5 23:30:41 2023 +1030
 1 file changed, 22 insertions(+)
[main 4b1a985554] fix: add indicator for copied
 Author: jakecastelli <[email protected]>
 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)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
tools: add button to copy code example to clipboard

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD 8e8700a770] tools: add button to copy code example to clipboard
Author: jakecastelli [email protected]
Date: Fri Mar 3 18:55:16 2023 +1030
2 files changed, 32 insertions(+), 2 deletions(-)
Rebasing (3/12)
Rebasing (4/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: keep setup function naming consistent

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD b457accb8b] fix: keep setup function naming consistent
Author: jakecastelli [email protected]
Date: Fri Mar 3 22:51:30 2023 +1030
1 file changed, 2 insertions(+), 2 deletions(-)
Rebasing (5/12)
Rebasing (6/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: improve copy button naming

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD 91e88e424d] fix: improve copy button naming
Author: jakecastelli [email protected]
Date: Fri Mar 3 22:54:25 2023 +1030
1 file changed, 4 insertions(+), 4 deletions(-)
Rebasing (7/12)
Rebasing (8/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: move copy button to the right

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD 0cc3e6f362] fix: move copy button to the right
Author: jakecastelli [email protected]
Date: Sat Mar 4 22:49:10 2023 +1030
1 file changed, 5 insertions(+)
Rebasing (9/12)
Rebasing (10/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: improve button style

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD c99de7088f] fix: improve button style
Author: jakecastelli [email protected]
Date: Sun Mar 5 23:30:41 2023 +1030
1 file changed, 22 insertions(+)
Rebasing (11/12)
Rebasing (12/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: add indicator for copied

PR-URL: #46928
Refs: #46894
Reviewed-By: Debadree Chatterjee [email protected]

[detached HEAD 4034f072a3] fix: add indicator for copied
Author: jakecastelli [email protected]
Date: Sun Mar 5 23:31:00 2023 +1030
1 file changed, 6 insertions(+)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/4518347599

@debadree25 debadree25 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2023
@nodejs-github-bot nodejs-github-bot merged commit 94ec71d into nodejs:main Mar 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 94ec71d

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: copy example code to the clipboard
8 participants