FEATURE: Add copy button to codeblocks (PR #9451)

This PR converts the copy codeblocks theme component to a core feature. The codeblocks are only shown when the show copy button on codeblocks site setting is enabled. This is defaulted to off.

Additionally, I’ve also fixed an issue where codeblocks inside of GitHub oneboxes have a copy button added. This uses the :scope selector, which is not compatible in IE 11. I did find a possible polyfill we could use but my knowledge is limited there.

GitHub

1 Like

no var, use let/const, I know it’s third party code, but we can’t have this in the codebase.

it’s usually better to do:

if (!siteSettings.show_copy_button_on_codeblocks) {
  return;
}

It avoids adding one un-necessary indentation level

I don’t think we want to be bothered by IE here, however, please test how bad it fails on IE please, if it just doesn’t show we don’t care.

Maybe add this in the site setting description, eg: not supported on IE.

1 Like

Now that I think about it, as it’s a core PR? if you want you can add https://github.com/jonathantneal/element-qsa-scope/blob/master/index.js in the discourse-internet-explorer ie.js file

@jjaffeux fixes should be wrapped up in the last push. Removed IE 11 support since the button was rendering without polyfill but failed to work in general.

    show_copy_button_on_codeblocks: "Add a button to codeblocks to copy the block contents to the user's clipboard. This feature is not supported on Internet Explorer."

I think we should hold a reference to later and cancel it in cleanup http://api.emberjs.com/ember/3.14/functions/@ember%2Frunloop/cancel

Let me know if you need help on this.

any reason we don’t use trim() here ? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trim

        const { isIE11 } = container.lookup("capabilities:main");

give $elem[0] a proper name so we can reuse it after

now that I think about it, I think we can just all have in one line:

clipboardCopy(code.innerText.trim());

I think I took care of this in the latest commit. Have a look to make sure I understood correctly.

I might be missing something here, but it seems to me like _clickHandlerElement could be overwritten here.

What if two posts have commands? The first would capture the element, and the second would overwrite it, which means it cannot be cleaned up.

To confirm this you could add an assertion here that throws an error if _clickHandlerElement is not null.

Oh yes!

Thanks robin, totally forgot this, this is for this use case I made this commit the other day: DEV: pass widget back when cleaning up stream (#9422) · discourse/discourse@6023ea1 · GitHub

I think it will allow to cleanly remove create event listeners for each post.

Added a patch thanks to @jjaffeux to clean this up.

@jjaffeux @eviltrout are we good to merge this now?

I hope that in the case when a post is edited to add/remove a comand we call cleanUp before decorating again because these would stamp on the other ids. Something to test!

OFC we don’t :stuck_out_tongue: good catch, will think about it…

Most recent commit (patch from @jjaffeux) should address this.