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

[docs] Add copy button to code block #32390

Merged
merged 18 commits into from
Apr 27, 2022
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 20, 2022

Preview: https://deploy-preview-32390--material-ui.netlify.app/material-ui/getting-started/installation/

Add a copy button to the code block in docs and blog pages

  • support mouse click
  • support focus
  • support cmd+C & ctrl+C when hovering on the code block
  • add GA event to compare with the existing copy button (if the new copy works better, we can remove the existing copy button in the future)
Screen.Recording.2565-04-20.at.23.39.52.mov

@siriwatknp siriwatknp added the docs Improvements or additions to the documentation label Apr 20, 2022
@mui-bot
Copy link

mui-bot commented Apr 20, 2022

No bundle size changes

Generated by 🚫 dangerJS against 5055ece

@siriwatknp siriwatknp marked this pull request as ready for review April 20, 2022 16:33
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, cool! This is a really handy functionality, I use it all the time for the terminal commands I have saved in my private Notion, awesome to see in the docs too!

I have only a couple of considerations for moving forward:

  • Regarding command code blocks like the one you recorded as example, should we use this PR to separate all the ones that have more than one command in the same block? What I mean is: I wouldn't want to copy the npm and yarn command at the same time.
  • Should we add on hover, alongside the "Copy" button, a "⌘C" as well? If you haven't told it on the PR description, I guess I'd have figured that out by accident only, and it's way handier than clicking!

@siriwatknp
Copy link
Member Author

  • Regarding command code blocks like the one you recorded as example, should we use this PR to separate all the ones that have more than one command in the same block? What I mean is: I wouldn't want to copy the npm and yarn command at the same time.

I think fixing the doc should be a separate PR (I assume that there are more than one file)

Should we add on hover, alongside the "Copy" button, a "⌘C" as well? If you haven't told it on the PR description, I guess I'd have figured that out by accident only, and it's way handier than clicking

I am not sure I get this. Do you mean when hovering, it will copy automatically? Can you explain the UX you have in mind?

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🎉

docs/src/modules/utils/useCodeCopy.ts Outdated Show resolved Hide resolved
@@ -1,30 +1,72 @@
/* eslint-disable material-ui/no-hardcoded-labels */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not translate 'Copied' and 'Copy'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. But I am not sure how we can translate the markdown generated by marked.js.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not translate this one. It is not consistent with the button generated by markedjs. We can revisit later when we have a better way.

color: '#fff',
backgroundColor: blueDark[600],
},
'&:focus-visible': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be strict, then we would need to use ButtonBase or our hook

Suggested change
'&:focus-visible': {
'&.Mui-focusVisible': {

https://caniuse.com/?search=focus-visible vs. https://mui.com/material-ui/getting-started/supported-platforms/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case, it is tricky. There are 2 places:

  1. the button generated at file system read (markdown file)
  2. the button created by React

It is possible to use ButonBase for the second one but I don't think the first point is possible, that's why I decide to go with focus-visible to keep the consistency. For a safer choice, we could go with :focus.

docs/src/modules/utils/CodeCopy.tsx Outdated Show resolved Hide resolved
docs/src/modules/components/Demo.js Outdated Show resolved Hide resolved
docs/src/modules/components/HighlightedCode.js Outdated Show resolved Hide resolved
docs/src/modules/components/HighlightedCode.js Outdated Show resolved Hide resolved
docs/src/modules/components/MarkdownDocs.js Outdated Show resolved Hide resolved
docs/src/modules/utils/CodeCopy.tsx Outdated Show resolved Hide resolved
@danilo-leal
Copy link
Contributor

@siriwatknp following up on my suggestion, this is what I tried to describe:

hover

Just add a "Press ⌘C" alongside the "Copy" button to make sure folks know that by doing so, you can copy the snippet just as you would by clicking the button. Alternatively, we could simply do the below, which is definitely wordier but maybe clearer?!

Screen Shot 2022-04-25 at 09 54 33

@siriwatknp
Copy link
Member Author

I think "Click to copy or press ⌘C" is too long. We should keep it short, "Copy" is the best I guess. Once the users know that they can press "⌘C" to copy, the text will become annoying. I propose that we show the text only when mouse is hovering on the copy button like this:

Screen.Recording.2565-04-26.at.09.56.22.mov

The video represents only the UX, the styles can be adjusted.

I have set up the analytics to track the keypress copy. We can revisit the UX again if my proposal does not work well. @danilo-leal What do you think?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2022
@danilo-leal
Copy link
Contributor

I like that! It's indeed way better than what I had previously suggested. Let's go for it!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2022
@m4theushw
Copy link
Member

Small detail but the button in https://master--material-ui.netlify.app/ doesn't look correctly.

image

@siriwatknp
Copy link
Member Author

Small detail but the button in https://master--material-ui.netlify.app/ doesn't look correctly.

image

Thanks, will create a hotfix.

@m4theushw
Copy link
Member

Turns out that @flaviendelangle already did it: #32638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants