-
Notifications
You must be signed in to change notification settings - Fork 64
Add copy-to-clipboard support to code blocks #961
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
base: main
Are you sure you want to change the base?
Conversation
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DebugSteven :) I'm really excited to see you bringing this functionality to DocC and DocC-Render, and I especially love the minimal/additive syntax proposed to make this a progressive, opt-in enhancement for DocC code listings.
Unfortunately, I'm about to be offline for a week, so I won't have time to properly test/review this PR during that time.
However, I took a brief look at the implementation and have a suggestion for a simpler way to position the icon/button that maybe you could explore—I promise to take a more complete look at this PR when I'm back online either way—sorry again for the bad timing on my end!
@@ -122,6 +170,43 @@ export default { | |||
line === '' ? '\n' : line | |||
)); | |||
}, | |||
updateCopyButtonPosition() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking there might be a simpler way to position this element using pure CSS that would eliminate the need for refs and any custom JS listening to events, etc
As a very basic example, this is some minimal CSS that could be used to position a small red box with the letter "A" in the top-right corner of a given code listing. Maybe you could check this out and explore a CSS based solution to the layout and position for the icon/button to avoid the need for scroll event listeners?
diff --git a/src/components/ContentNode/CodeListing.vue b/src/components/ContentNode/CodeListing.vue
index 8f1524fc..ba7ceebc 100644
--- a/src/components/ContentNode/CodeListing.vue
+++ b/src/components/ContentNode/CodeListing.vue
@@ -129,6 +129,19 @@ export default {
<style scoped lang="scss">
@import 'docc-render/styles/_core.scss';
+.code-listing::after {
+ align-items: center;
+ background: red;
+ color: white;
+ content: 'A';
+ display: flex;
+ height: 25px;
+ justify-content: center;
+ position: absolute;
+ right: 0;
+ width: 25px;
+}
+
.code-line-container {
display: inline-block;
width: 100%;
(The important pieces there being the position: absolute
and right: 0
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this suggestion! I'll try to get the button positioning working with pure CSS and I'm glad it's feasible. I was concerned about the performance implications with the scroll event listeners.
51054a0
to
14914c2
Compare
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great! I'm glad that the absolute positioning approach simplified things and is working out well.
I have some feedback after looking this over more closely and testing it out, but my comments are mostly pretty minor suggestions.
Thank you so much for looking into this!
@click="copyCodeToClipboard" | ||
aria-label="Copy code to clipboard" | ||
> | ||
<svg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could extract both of these icons to their own Vue components, similar to how it has been done for other svg icons.
There isn't anything wrong with how you've done it inline here, but with the components we could more easily re-use them in other places at a later point, and we could introduce a customization point so the actual assets would be easier to swap with theming as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated the icons into separate components like you asked. However, I couldn’t get them to work with the SVGIcon
component without messing up the CSS. Is there a trick I'm missing?
class="copy-button" | ||
:class="{ copied: isCopied }" | ||
@click="copyCodeToClipboard" | ||
aria-label="Copy code to clipboard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the "Copy code to clipboard"
string to a new key-value pair in src/lang/locales/en-US.json
so that we could more easily translate this text for other languages in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the string to en-US.json, but I’m not entirely sure it’s added correctly. Could you double check and adjust if needed or let me know the correct approach?
}, 1000); | ||
}) | ||
.catch(err => ( | ||
console.error('Failed to copy text: ', err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty minor detail since I think this API is pretty well supported at this point, but I wonder if we should also show a failure icon in the UI to make this failure less silent/hidden?
Just pseudo code, but maybe it could be something like:
async copyCodeToClipboard() {
try {
await navigator.clipboard.writeText(copyableText);
this.copyingState = CopyingState.succeeded;
} catch (err) {
console.error(...);
this.copyingState = CopyingState.failed;
} finally {
this.setTimeout(() => {
this.copyingState = null;
}, duration);
}
}
(The state could then just be used to toggle which version of the icon to show at any given time)
This probably isn't a blocking issue since the copy functionality is pretty widespread at this point and unlikely to fail—just thinking out loud mostly.
position: absolute; | ||
top: 0.2em; | ||
right: 0.2em; | ||
width: 24px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially we could size this using em units like 1em
to have the icon or button size match the size of the code listing text itself so it scales as users adjust the font-size on their browsers.
I think this size looks pretty good though if we want to leave it hardcoded to 24px—just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the width/height to be 1.5em because I thought that would be equivalent to 24px. Let me know if you were thinking of doing this differently.
@@ -22,6 +22,33 @@ | |||
>{{ fileName }} | |||
</Filename> | |||
<div class="container-general"> | |||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design-wise, I think the button can be a little distracting for code listings with a long first-line of code since the button always shows directly over the text (when enabled).
If we were to enable this button by default for code listings (which I personally think would be great), we may want to consider only showing it when hovering over the listing to try and eliminate some of that distraction maybe? That's just my personal opinion though. We could also consider a different layout where the button doesn't appear directly over the text as an alternative, although the positioning could get tricky in that case.
As a concrete example, 2 of the 3 code listings on this screenshot from the PR description are obscured by the button, regardless of where the user focus is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it’s nicer to show the copy button only when hovering over code blocks. Originally I had the button only show when hovering over code blocks. However, Joe and I got some feedback on the forums that it’d be better for mobile use to have the button always visible.
I think I’ve got a decent solution here in my latest commit. I’m using @media (hover: hover)
for devices that support hover and I added @media (hover: none)
, when hover isn’t supported, to set the button to be always visible. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
This PR adds support for a new copy-to-clipboard button in code blocks. When this feature is enabled through the
enable-experimental-code-block
flag, a copy-to-clipboard button is rendered in the top-right corner of all code blocks, allowing users to easily copy its contents. When an author does not want the contents of a code block to be copyable, anocopy
option can be used to disable the copy-to-clipboard button.User Experience
When the
enable-experimental-code-block
flag is used, a copy button will appear in the top-right corner of all code blocks. Clicking the button copies the full contents of the code block to the clipboard and displays a checkmark to confirm success. If a code block includes thenocopy
keyword in the language line, the copy button will not appear on that code block.Implementation Overview
swift-docc-render
, this addscopyToClipboard
to the properties onBlockType.codeListing
.CodeListing.vue
, similar to swift.org’s copy-to-clipboard button on its code blocks, whencopyToClipboard
is set to true from the feature flagenable-experimental-code-block
. Includes a function,copyCodeToClipboard
, to copy the contents of a code block to clipboard.```nocopy
disables the copy button on that code block.swift-docc
. The accompanying branch/PR is here: Add copy-to-clipboard support to code blocks swift-docc#1273Dependencies
This PR depends on associated changes in
swift-docc
to pass the parameter for the copy button.Testing
Setup
swift-docc
andswift-docc-render
with the copy-to-clipboard changes.swift-docc
with the feature flagenable-experimental-code-block
and serve it using a local build ofswift-docc-render
.How to Test
enable-experimental-code-block
flag, a copy button will appear in the top-right corner.To disable the copy icon with the feature flag enabled:
```
or by adding a code listing, add thenocopy
option like this:or like this:

2. Verify the copy button does not appear on this code block.
Checklist
npm test
, and it succeededRenderNode.spec.json
to includecopyToClipboard
as a property onCodeListing
. Please let me know if there's any other documentation I should update here.