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 to clipboard functionality #339

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

danditomaso
Copy link

This adds the copy to clipboard feature that is available on many documentation websites. It sits inline with the text, so there wont be any cases where it will overlap existing content.

Clicking the copy button takes the contents inside the code block and copies it to your clipboard, you receive a tooltip when clicking it that disappears after a second. I'm open to tweaking the timing on the tooltip if it feels slow.

@francislavoie
Copy link
Member

Hmm, this had some side-effects on /docs/install. For example, the Debian section's commands have a lot more padding now.

Before:

image

After:

image

Also I'm only seeing the copy button on some of the blocks on this page. I'm not sure why though, they aren't significantly different other than missing the button 🤔 Probably just a selector bug.

Comment on lines -136 to -137


Copy link
Member

Choose a reason for hiding this comment

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

These extra empty lines were kinda intentional to split apart the file in sections. We should probably add comments between these sections instead I figure.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'm happy to add a note here, do you know what note I should add? It looks like more general selectors are above and more specific ones are below, but I'm guessing.

});

// select all bash code elements and dynamically add clipboard button/svg
$("pre > code.bash").map(function (_, block) {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, you only added it to code.bash, I see. I think in some cases it's code.cmd > span.bash as well. Or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I've reduced the specificity of the selector and added span.bash.

@francislavoie francislavoie added the enhancement New feature or request label Aug 11, 2023
@danditomaso
Copy link
Author

danditomaso commented Aug 14, 2023

Hmm, this had some side-effects on /docs/install. For example, the Debian section's commands have a lot more padding now.

Before:

image

After:

image

Also I'm only seeing the copy button on some of the blocks on this page. I'm not sure why though, they aren't significantly different other than missing the button 🤔 Probably just a selector bug.

@francislavoie I'm seeing the same thing, I've added some changes to fix that in other locations. I think a better fix for this is for any case where there is a code element along with more than one span, is to add an inner div, keep the cmd's within that inner element along with some padding. Then absolutely position the copy button outside of that code block, which will ensure it wont overlap any text. I wont have a chance to update this PR with thos fixes until late afternoon today if that is alright?

@francislavoie
Copy link
Member

No rush at all 👍 take your time

@mholt
Copy link
Member

mholt commented Aug 14, 2023

Yeah, thanks for working on that! No problem, we're not in a hurry :)

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, Dan! It looks good to me. :) Should be a great boon to our readers.

@mholt
Copy link
Member

mholt commented Sep 7, 2023

Is this ready to merge? If not, feel free whenever it's ready :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants