-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(www): add copy button to code snippets #15834
Conversation
www/src/components/pre.js
Outdated
const [language, { title = `` }] = getParams(children.props.className) | ||
const content = children.props.children | ||
return ( | ||
<Highlight |
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.
Note: we’ll probably want to pass our custom Prism theme as a theme here — will do just that!
Just stopping by to say that this is incredible @DSchau 💜 |
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.
Oh @DSchau you glorious soul, this trophy is for you: 🏆
This is so awesome, this has been an open issue for ages, so glad someone just jumped in to hammer it out. I know @marcysutton had some concerns about accessibility but this is looking great to me! (her success criteria was here on the issue: #5030 (comment))
@marcysutton @gillkyle could use some assistance! Let me address what I’ve tried to improve re: accessibility, and then we can see if there’s room for improvement!
✅
I used the text
Maybe done? I used the text you describe as the
✅
✅
I didn’t do this because we don’t really have a pattern for this — and it’s a bit out of scope for this PR. I can do this, but I’m thinking swapping the text could be enough? More I can do here?
5 seconds? More or less?
✅ I think. I stole your ScreenReaderText component! |
<3 Will have a mock for this by tomorrow. |
I think swapping the text as a notification might actually simplify some of the accessibility concerns. I recall @marcysutton talking about how toast notifications can be difficult with focus though she'll certainly have a more definite opinion. |
Woo-hoo! I wrote some relatively janky-ish code to add support for “directives” (i.e. highlight-start, highlight-end, hide-line, etc.), and the accessibility seems to be much approved — but still could use a once-over from someone a little more well-versed than myself. I think this is ready for a set of 👀 Still need that slick design tho @fk but no rush! Longer term, we should probably open-source this as a separate package; I can imagine it being pretty useful in any MDX setup! |
(I thought I commented on this yesterday, but didn't hit send!) Yay, that all sounds good to me! I'm so glad I wrote those success criteria. After user testing I actually agree that putting the "copied" text right on the button makes a lot of sense. I wouldn't worry about changing the design for that, but we might need to do a little more to make sure the "${filename} copied" message is announced in a screen reader. Thanks so much for working on this! |
Are we using the same code highlighting component as in gatsby-theme-blog? |
@KyleAMathews nope. That one is It is fairly similar, but we’re not using theme-ui (yet…) on .org, so I think it makes some sense to have a separate component here. |
AFAIK the UI needs to support four different scenarios:
I'm assuming we want to have a "Copy" button for every block of code.
This might be the most common scenario in our docs, but let's take a look at some more, e.g. code block showing just one line of code.
This is not super-duper touch friendly though (irt the button size), but would probably be an acceptable tradeoff (assuming that the majority of our users are on a desktop-like device).
If we'd want the "Copy" button to look more like a button, we could decrease the overall padding, and slightly increase the offset to the top and right: |
@fk implemented! 🙌 |
This should be ready to go! 🚀 |
@marcysutton for sure! @fk and I were definitely inspired by Bootstrap here, but if we need it to be more button-y (that’s a word, trust me), we can do that too! |
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 think this is incredible and ready to ship so leaving this approval here! Not hitting the big green button just yet in case we want to handle this
but I'm wondering if users will look at "Copy" in text and assume it's not interactive
Going to merge! Thanks for the thoughtful review, comments, and design details everyone 🚀 |
Thanks for merging Dustin! We can follow up with tweaks and stuff...once it's deployed on the site I'll take a look and see if I have any recommendations to chat through with @fk. This is going to be so helpful for users! |
@DSchau You are killing it! Any plans to publish it as a separate package? I will update gatsby-remark-code-buttons README.md to link to this package. |
@DSchau How is this implemented? I can't seem to find documentation anywhere on how enable a copy button? I see the functionality on this page: https://www.gatsbyjs.org/docs/use-static-query/ but haven't been able to find what to do to use it |
@iamskok definitely on the roadmap! What we’d probably like to do is enhance the theme-ui component to include these line highlighting changes, as well! @Blackglade sorry — it’s in this PR? You should be able to check out the files changed to discover more? As far as using it — click the button and it’ll copy to your clipboard! |
for a alternate, fully featured approach see gatsbyjs/gatsby#15834
children, | ||
className = children.props ? children.props.className : ``, | ||
}) => { | ||
const [language, { title = `` }] = getParams(className) |
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.
@DSchau I love it 😍 and plan to use some of that to add that nice copy feature to coding4.gaiama.org too 👍 🚀
By the way mdx
passes additional space separated props as well so you could get the title
from props.title
if you pass the title like so
```js title=gatsby-config.js
instead of
```js:title=gatsby-config.js
then it won't end up in className and you wouldn't have to extract
Yet it's working as is and would be quite a pain to migrate all docs I guess 😅
Update: just realized this PR is quite old and my comment might've not worked back in the days ^^
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.
FYI: i found your described behaviour here in the mdx docs:
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.
Uh yeah thanks for the link that's where I know this from 😁🙏
Description
At a workshop yesterday, @KyleAMathews and I noticed that people tend to highlight the wrong lines, or lose a few lines when highlighting leading to strange errors and confusion. We can avoid this by introducing a copy functionality to our code snippets.
This PR does just that while also maintaining most of our other features, notably:
jsx:title=gatsby-config.js
)title
as a JSX prop or somethingA
gif
is worth lots of words, so it looks something like this:Also @fk would love your design eye — I think it looks OK but could certainly be improved :) Feel free to tweak anything!
Related Issues
Fixes #5030