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

Add copy button for code snippet #3281

Merged
merged 6 commits into from Mar 29, 2022
Merged

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Mar 22, 2022

Issue This PR Addresses

Fixes #2923

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Image 2022-03-27 at 11 50 PM 2
Image 2022-03-27 at 11 50 PM

Steps to test the PR

  • pnpm i
  • cp config/env.staging .env
  • pnpm dev

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 22, 2022

@humphd
Copy link
Contributor

humphd commented Mar 22, 2022

In our meeting today we discussed copying how GitHub does this, so that you only see the "copy" button when you mouse over the area, and it is removed when you leave that area. We should see what they do with touch events for mobile.

Also, I wonder if we should steal their icon too vs. using the "copy" text?

Copy link
Contributor

@DukeManh DukeManh left a comment

Choose a reason for hiding this comment

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

Instead of listening to the mouse move, you can use mouseenter/mouseleave event handler. I suggest that you do the followings:

  1. const snippets = post.querySelectorAll('pre code')
  2. snippet.onmouseenter = () => { const button = createButton(); button.onclick = () => copySnippet(snippet.text); snippet.appendChild(button); }
  3. snippet.onmouseleave = removeButton

src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
src/web/src/pages/CopyButton.css Outdated Show resolved Hide resolved
@TueeNguyen
Copy link
Contributor

@menghif, I was playing around with this PR, trying to make mouseenter & mouseleave to work. I opened a PR in your fork

@AmasiaNalbandian
Copy link
Contributor

Hello! Please rebase, as the following commit 6046273 has reorganized our file structure!

@menghif
Copy link
Contributor Author

menghif commented Mar 24, 2022

This now shows/hides the button as expected

@TueeNguyen
Copy link
Contributor

It's so cool to see you took some inspiration from me 😄

@menghif
Copy link
Contributor Author

menghif commented Mar 25, 2022

It's so cool to see you took some inspiration from me 😄

I put you as a co-author 😊

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I like this, but I don't think the code is ready.

src/web/app/src/components/Posts/Post.tsx Show resolved Hide resolved
@DukeManh
Copy link
Contributor

DukeManh commented Mar 26, 2022

@menghif I know a bit about Posts so I want to collaborate with you on this here menghif#7

I added fade in and out button transition, a copy icon like GitHub/Mozilla does it. And I moved the listener to a higher level component so it doesn't get called for each post.

humphd
humphd previously approved these changes Mar 28, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Nice.

One thing I notice during testing is that we now have two copy buttons, and they use different colours (new one is dark, old one is light):

Screen Shot 2022-03-28 at 9 08 02 AM

Can we file a follow-up to make them the same?

Copy link
Contributor

@AmasiaNalbandian AmasiaNalbandian left a comment

Choose a reason for hiding this comment

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

This got rid of my previous feedback - I'm also loving the instant feedback that the click has registered and has copied the code block! Nice work team!

Copy link
Contributor Author

@menghif menghif left a comment

Choose a reason for hiding this comment

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

This looks amazing! Thank you @DukeManh for making it great

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd like to see an issue filed to deal with the difference between how the two copy buttons work. We should re-use the same component or at least have the styles be the same.

Copy link
Contributor

@DukeManh DukeManh left a comment

Choose a reason for hiding this comment

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

Filed #3356 follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a code snippet copy button
6 participants