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

Copy code to clipboard #2812

Merged
merged 12 commits into from
May 5, 2024
Merged

Copy code to clipboard #2812

merged 12 commits into from
May 5, 2024

Conversation

iBug
Copy link
Collaborator

@iBug iBug commented Feb 6, 2021

Add a "copy to clipboard" button for each <pre> block in main body. Code from my website repo with small non-functional adjustments.

Live preview available:

Screenshot:

image

Caveat: There's only one single color written for the button because I assumed that all skin variants use "dark mode" for blocks of code (<pre> blocks).

More testing and appearance tweaking is recommended before merging.

Contexts

This is a very popular feature and requested multiple times.

#2338 #2508 #2766 #2795

Some technical notes

My approach does not depend on code blocks having IDs, but instead searches for the containing code block starting from the button. This is not resistant to structural changes, e.g. when an adjacent HTML tag is changed or the button is wrapped in another layer of <span> or whatever. (but who does that, really?)

The CSS selector I used is .page__content pre > code (and the go back a level to locate the <pre> element), which should be safe from unexpected HTML structures while covering most regular code blocks.

The actual "copy" action is done via document.execCommand("copy"), which should have the best compatibility according to this answer (quoted below), yet it's deprecated in 2020.

Browser Support

The JavaScript document.execCommand('copy') support has grown, see the links below for browser updates:

Credits

@iBug really wants a place here in the Credits section :P
carlesfernandez added a commit to gnss-sdr/geniuss-place that referenced this pull request Feb 9, 2021
Add code based on mmistakes/minimal-mistakes#2812 with some custom changes:

Add the option to not creating the copy button by using a “nocopy” class

Remove bash prompt symbol when copying instructions, so they can be directly pasted into a terminal

Update javascript packages

Tested on Chrome, Safari, Opera, Firefox

Change some three back-ticks by one when inlining
@carlesfernandez
Copy link

Hi @iBug,

I've been playing with this code and works nice. I have some humble (and surely naive) suggestions:

1.- Sometimes, it makes no sense to copy the code block. For instance, the output of a program in a Terminal. So I would like to be able to shut down the "Copy to clipboard" button in certain code blocks.

For instance, adding something like:

https://github.com/gnss-sdr/geniuss-place/blob/225737d40e73a9012a8d08898e66a1bd0f8d8f7e/assets/js/_main.js#L236-L239

So I can do:

```
Nobody wants to copy this text to the clipboard
```
{: class="nocopy"}

... but I'm sure there are more elegant ways to do that.

2.- The implementation throws an Uncaught TypeError: Cannot read property 'querySelectorAll' of null when the page does not have any code block. This change seems to fix it:

var elem = document.querySelectorAll(".page__content pre > code");
  if (elem) {
    elem.forEach(function(element, index, parentList) {
       // Locate the <pre> element
       ...

3.- Since document.execCommand("copy") is deprecated, maybe it's better to have it as a fallback if navigator.clipboard is not available:

try {
      textarea.select();
      if (document.queryCommandEnabled('copy') && navigator.clipboard) {
        navigator.clipboard.writeText(textarea.value);
      } else {
        success = document.execCommand("copy");
      }
    } catch (e) {
      success = false;
    }

It seems that navigator.clipboard is present in recent versions of Chrome, Firefox, Safari and Opera.

@iBug
Copy link
Collaborator Author

iBug commented Feb 9, 2021

@carlesfernandez

  1. It's a good suggestion, though I went for the CSS approach which should provide more compatibility (IE doesn't support Element.closest)

    Preview: https://ibug.github.io/minimal-mistakes/docs/utility-classes/

  2. It's some other code causing this error, not the part you pointed out. There's nothing to be done to that, however.

    document.querySelector('.page__content').querySelectorAll('h1, h2, h3, h4, h5, h6').forEach(function(element) {

  3. Accepted and I refactored the code so when taking the navigator.clipboard path the entire "create <textarea> element" logic is bypassed, improving performance.

assets/js/_main.js Outdated Show resolved Hide resolved
@luispuerto
Copy link
Contributor

Hey!!!

Really great!!! I really wanted to add something like this to my site!! Thanks a lot!

@luispuerto
Copy link
Contributor

ummm… the feature is neat, but only works once in a page, after that you have to reload to copy again form another codeblock.

@luispuerto
Copy link
Contributor

Question… why don't you use this?

@iBug
Copy link
Collaborator Author

iBug commented Feb 11, 2021

@luispuerto

ummm… the feature is neat, but only works once in a page, after that you have to reload to copy again form another codeblock.

It's more likely there's something wrong with your copy of the code. The behavior in the demo links on ibug.github.io should be in line with what this PR would bring to the theme.

Question… why don't you use this?

Quoting @mmistakes here:

Only drawback I could see is AnchorJS is way more JavaScript (~350 lines unminified) than @iBug's solution, which is about 10 lines unminified.

This is a similarly simple feature that simpler code would be easier to maintain.

@luispuerto
Copy link
Contributor

luispuerto commented Feb 11, 2021

It's more likely there's something wrong with your copy of the code. The behavior in the demo links on ibug.github.io should be in line with what this PR would bring to the theme.

I swear over ms-dos and unix that even your demo is behaving like that. In safari. On Firefox I can't even make it work. On macOS Big Sur and last versions of all the apps involved. I even disabled content blockers and so just in case.

  • On safari only worked once and if I want to make it work again I need to reload the page.
  • On Firefox doesn't work at all.
  • On Chromium works.

Quoting @mmistakes here:

Only drawback I could see is AnchorJS is way more JavaScript (~350 lines unminified) than @iBug's solution, which is about 10 lines unminified.

I totally understand and see you there. That extension is used for several sites like github, or for example for R documentation sites like this one.

I much prefer your solution to be honest, if finally works. Specially because I like how you implemented the CSS and it's already a complete solution.

This is a similarly simple feature that simpler code would be easier to maintain.

I really don't know about that, but wouldn't be easier just to update the extension if it continue to be maintained?

@iBug

This comment has been minimized.

@iBug
Copy link
Collaborator Author

iBug commented Feb 11, 2021

@luispuerto It's a stupid typo from somewhere I couldn't recall. Can you test the demo pages now on Safari? It's working in Firefox for me now. Make sure to clear your browser's cache for the updated JavaScript.

@luispuerto
Copy link
Contributor

@iBug now works! 👍🏻 thanks a lot! 🚀

luispuerto pushed a commit to luispuerto/luispuerto-net that referenced this pull request Feb 12, 2021
iBug referenced this pull request May 13, 2021
@iBug
Copy link
Collaborator Author

iBug commented May 13, 2021

@mmistakes Given the discussions here and on #2795, #2508 and #2338, as well as that (at least) two people also ported code from this PR into their own repositories, I think there's enough popularity for this feature to get included. Any ideas?

@mmistakes
Copy link
Owner

I have usability and accessibility concerns with how it's currently implimented.

  1. No focus or active states. Hard to tell if you can navigate to the button by keyboard and trigger the copy function.
  2. No text label to indicate what the icon even does.
  3. No feedback that it's done anything after click/tap... e.g. "Copied!" or something to that affect.
  4. Any UI text used should probably be included in the _data file so it can be localized and/or changed easily.

Then there's the concern of turning this on by default. I could see users not wanting it at all and now we've made it that they need to add a class to every code block to disable. I'd prefer it be optin globally rather than optout.

This would also prevent loading more JavaScript for a feature not used by someone. Though it might be more of a pain to try and conditionally add those scripts to the page(s) which gives me pause on this PR as well.

@iBug
Copy link
Collaborator Author

iBug commented May 17, 2021

Addressing one concern quickly:

they need to add a class to every code block to disable

With the current implementation it's not. There's a rule for @at-root .no-copy & so one could just add .no-copy to any parent element (preferably <body>).

Making this an opt-in feature is similarly easy: Require a "copy-enabled" class for a parent element, then there could be a switch to add that class to <body>.


Plus, I'm a bit busy with RL stuff recently, and I'll get back to this PR in maybe June (half a month or so later).

@mmistakes
Copy link
Owner

Still hesitant on this one. I can already see the feature requests asking for a config flag to turn it on/off globally.

@iBug
Copy link
Collaborator Author

iBug commented May 17, 2021

OK, I'll implement that global toggle flag with two modes: Global enable + individual page or block opt-out / global disable + individual page or block opt-in.

One technical question: Since the elements and all their HTML content are produced from JS, how should I integrate i18n into that? There's no direct way to transfer data from _data/something.yml to JS.

@mmistakes
Copy link
Owner

mmistakes commented May 17, 2021

Another global flag is exactly what I’m trying to avoid.

The point I was trying to make was I’m hesitant on adding this feature. Not because it doesn’t add value, but because it’s the sort of thing I expect users to not be happy with the implementation and want ever more customization.

  • I want to turn on code copy for all blocks
  • I want to turn it off on code blocks on X post
  • I want it on this block but not that block
  • I don’t want to use a Kramdown attribute to enable/disable
  • Etc

I’d rather leave it to the user to override the theme if they want this feature vs the theme trying to do something core Jekyll or a plugin should do.

Even better this probably should be an enhancement to Jekyll’s highlight tag. Similar to how it allows enabling for line numbering.

@iBug
Copy link
Collaborator Author

iBug commented May 17, 2021

Jekyll's highlight tag is well beyond the scope of a theme (it's Ruby), but for the other concern,

expect users to not be happy with the implementation and want ever more customization

Users have always been wanting ever more customization as is evident by the amount of Issues you're receiving, not to mention some of them are fantastical.

I propose this feature for integration into the theme most simply because it's been requested multiple times and has the popularity in the user base. There are ones who have already gone one step sooner and customized on their own. And there certainly are more who want it badly but don't have the skill to push that. 99% of the users will have the attitude that "I'll happily enable it (or stay with it) if it's ready and easily reachable".

And there will be users content or discontent for every single feature, so that argument pretty much applies to everything. IMHO, the rule of thumb should be "if the majority goes fine with it, then it's fine". I believe it's courteous enough to provide a way to turn it off (and it's easy and documented).

@mmistakes
Copy link
Owner

mmistakes commented May 26, 2021

Another option would be to go the "helper" _includes path that I've done with other opt-in features like galleries, feature rows, etc.

I like the following solution where you drop in an include before a code block you want to add copy/paste support. I think this would address most of my concerns with the current implimentation.

https://www.aleksandrhovhannisyan.com/blog/how-to-add-a-copy-to-clipboard-button-to-your-jekyll-blog/

Instead of Emoji icons I'd leverage Font Awesome, make sure the text strings are done in a way that they can be customized via the UI text data file, add a parameter for filepath (optional), and maybe come up with a better include name than code header.

@iBug
Copy link
Collaborator Author

iBug commented May 26, 2021

Is there a good way to cover both "enable on individual code blocks" and "enable globally"? I'm still thinking of a global "enable" flag (unset by default so the current behavior remains).

@mmistakes
Copy link
Owner

Can't think of anything that would make a global enable doable.
I'm still against that. My view is the theme shouldn't do it, I only suggested the include as a compromise.

This is the sort of thing I think should be a plugin/enhancement to the {% highlight %} tag. For example I like how Gatsby does it with Prism remark plugins that extends the core:

https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-remark-prismjs
https://github.com/DSchau/gatsby-remark-code-titles

I could see the highlight tag being enhanced or forked to do something similar:

  • highlighting specific lines
  • add a code title/filename
  • enable copy/paste
  • enable line numbering

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

This pull request will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

@github-actions github-actions bot closed this Aug 3, 2021
@iBug iBug reopened this Apr 23, 2024
@iBug
Copy link
Collaborator Author

iBug commented May 5, 2024

As of the last commit Michael's questions are addresses as such:

  1. No focus or active states. Hard to tell if you can navigate to the button by keyboard and trigger the copy function.

The theme already has CSS for button:focus, though :active is missing.

I've tested with both latest Edge and Firefox and can confirm that it works with keyboard navigation.

  1. No text label to indicate what the icon even does.

Added <span class="sr-only">Copy code</span>.

  1. No feedback that it's done anything after click/tap... e.g. "Copied!" or something to that affect.

There are design and implementation concerns so not easy to address quickly... so maybe later?

Again, Material for MkDocs is a really good reference design for all these kinds of features. I may come up with something sensible in the future.

  1. Any UI text used should probably be included in the _data file so it can be localized and/or changed easily.

The only UI text is the sr-only span tag. Since the theme isn't localizing SR text for a few places (example), I'm not going to take the blame right here.


Most importantly, I would directly dismiss Michael's primary concern of whether users want this feature, as pretty much every, single, theme

Correction: Just The Docs has a site-wide switch, which is enabled by default though. The current approach (using uglify-js to squeeze everything into a single main.min.js doesn't allow us to follow that implementation easily.

Now that only point 3 is left completely unaddressed, I'm going to green-light this PR so as not to let this nice feature be blocked for too long.

@iBug iBug merged commit 0527e17 into mmistakes:master May 5, 2024
1 check passed
@iBug iBug deleted the copy-code branch May 5, 2024 11:43
@iBug
Copy link
Collaborator Author

iBug commented May 5, 2024

  1. No feedback that it's done anything after click/tap... e.g. "Copied!" or something to that affect.

1

This will be shipped along with the next release (4.26.1 or 4.27.0).

I'm no frontend web designer nor engineer, so the implementation is quite primitive. Anyways, better than none.

ElponeNote added a commit to ElponeNote/ElponeNote-github.io that referenced this pull request Jun 13, 2024
duetosymmetry pushed a commit to duetosymmetry/web-site that referenced this pull request Aug 15, 2024
Add "copy to clipboard" button for code blocks (mmistakes#2812)

* Add copy-to-clipboard button and JS

* Ignore line numbers if present

* Rewrite heading permalink code to use vanilla JS

* README: Add credits to zenorocha/clipboard.js (MIT License)

@iBug really wants a place here in the Credits section :P

* Add .no-copy for hiding the button, update docs

* Add td.rouge-code to selectors

* Fix navigator.clipboard branch

* Add screenreader text for copy button

* Restore focus to the button after copying

* Add site-wide enable switch
minyoongi96 pushed a commit to minyoongi96/minyoongi96.github.io that referenced this pull request Aug 26, 2024
* Add copy-to-clipboard button and JS

* Ignore line numbers if present

* Rewrite heading permalink code to use vanilla JS

* README: Add credits to zenorocha/clipboard.js (MIT License)

@iBug really wants a place here in the Credits section :P

* Add .no-copy for hiding the button, update docs

* Add td.rouge-code to selectors

* Fix navigator.clipboard branch

* Add screenreader text for copy button

* Restore focus to the button after copying

* Add site-wide enable switch
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.

4 participants