minimal-mistakes icon indicating copy to clipboard operation
minimal-mistakes copied to clipboard

Copy code to clipboard

Open iBug opened this issue 4 years ago • 21 comments

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 avatar Feb 06 '21 09:02 iBug

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.

carlesfernandez avatar Feb 09 '21 11:02 carlesfernandez

@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.

    https://github.com/mmistakes/minimal-mistakes/blob/116e29ad676ea81103a6c238a1f680767d738dab/assets/js/_main.js#L125

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

iBug avatar Feb 09 '21 14:02 iBug

Hey!!!

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

luispuerto avatar Feb 11 '21 10:02 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.

luispuerto avatar Feb 11 '21 11:02 luispuerto

Question… why don't you use this?

luispuerto avatar Feb 11 '21 12:02 luispuerto

@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.

iBug avatar Feb 11 '21 15:02 iBug

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?

luispuerto avatar Feb 11 '21 15:02 luispuerto

@luispuerto Thank you for your report. I've reproduced the error in Firefox and I'm looking into it. I don't have Safari, however.

iBug avatar Feb 11 '21 17:02 iBug

@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.

iBug avatar Feb 11 '21 18:02 iBug

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

luispuerto avatar Feb 11 '21 18:02 luispuerto

@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?

iBug avatar May 13 '21 05:05 iBug

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.

mmistakes avatar May 13 '21 12:05 mmistakes

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).

iBug avatar May 17 '21 01:05 iBug

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

mmistakes avatar May 17 '21 02:05 mmistakes

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.

iBug avatar May 17 '21 02:05 iBug

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.

mmistakes avatar May 17 '21 03:05 mmistakes

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).

iBug avatar May 17 '21 03:05 iBug

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.

mmistakes avatar May 26 '21 12:05 mmistakes

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).

iBug avatar May 26 '21 13:05 iBug

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

mmistakes avatar May 26 '21 13:05 mmistakes

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[bot] avatar Jul 26 '21 02:07 github-actions[bot]

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 avatar May 05 '24 11:05 iBug

  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.

iBug avatar May 05 '24 17:05 iBug