Add copy code button to code snippets #945

simonebortolin · 2022-08-31T16:36:27Z

Hello everyone, this is my implementation for the copy button on the snippet (requested in #924)

The implementation is made 100% javascript as with or without a jekyll template modification you still have to execute some javascript code, and I consider it the best choice.

the button only appears if the mouse is over it, to allow the entire line to be read

the important CSS changes were made to make the copy button work even in the long code situation:

to avoid this:

mattxwang · 2022-09-02T05:12:07Z

Hey @simonebortolin, thanks for submitting your PR! I'll give it a more in-depth look soon; in the meantime, could you resolve the lint errors?

simonebortolin · 2022-09-02T07:05:26Z

Yes of course the lint errors I wanted to solve them asap.

…

On Fri, 2 Sept 2022, 07:12 Matt Wang, ***@***.***> wrote: Hey @simonebortolin <https://github.com/simonebortolin>, thanks for submitting your PR! I'll give it a more in-depth look soon; in the meantime, could you resolve the lint errors? — Reply to this email directly, view it on GitHub <#945 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGMZW4EODCQCRPAWELTGJXTV4GEDDANCNFSM6AAAAAAQBRPPSM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

simonebortolin · 2022-09-03T19:48:35Z

Hello everyone, I should have corrected the style to make the lint happy (I hope, I have not yet seen the status of the automatic CI/CD controls)

I've also added an option to disable the copy button in the yml, I'm not sure if the logic is correct or if it's better != false for the default case management

simonebortolin · 2022-09-03T19:59:08Z

I will add some thoughts/information

does it make sense to put a tooltip saying "copy made successfully", in my opinion no, it is very efficient the tick that appears and since the theme is very simple it does not make sense to complicate it even more with a tooltip, popper library and the like
the icons come from bootstrap icons, licensed by MIT, I don't think anything else needs to be added

~~3) why did I use the spread operator? since both the spread operator and the forEach on nodelist should be supported on all modern browsers with ES6.~~
~~No reason, it's personal taste, I find the syntax with the spread operator much nicer, of course I can remove it if you prefer without it.~~
I also know that 60% of programmers use it without the spread (https://twitter.com/chriscoyier/status/1060235806709735424)

4) is the onclick on copybutton ugly? de gustibus, it could be improved a lot by saving the timeout instance and checking if there is already one, and not allowing to make two copies at short distance, but does it make sense? I have no idea, you tell me, I prefer to keep it as simple as possible.

~~5) the value of the fourSeconds variable could well be moved to yml, it's something I've thought about a lot and I'm in favour of doing it, maybe I'll put it in the next commit of changes.~~

P.S. If you look at my fork I am doing a total review of colour management and add form layouts, and I will submit them to PR as soon as they are ready, they are not ready yet and I am still creating them according to the needs of the sites where JTD is

simonebortolin · 2022-09-05T10:10:21Z

I have just realised that I have broken the numbered lists for lines of code, I will correct this as soon as I can.

mattxwang · 2022-09-06T19:01:31Z

Hello everyone, I should have corrected the style to make the lint happy (I hope, I have not yet seen the status of the automatic CI/CD controls)

FYI - you can run these locally as well with npm test, and you can autofix spacing, etc. with --autofix and/or --fix for Stylelint, Prettier, etc.!

I have just realised that I have broken the numbered lists for lines of code, I will correct this as soon as I can.

No worries, take your time!

mattxwang · 2022-09-15T00:16:40Z

Hey @simonebortolin, just wanted to check - were you hoping to merge this in for the upcoming release? If so, I think there's a merge conflict / CI issues, but after those are resolved happy to give a review!

simonebortolin · 2022-09-15T06:55:28Z

Hi, after September 21 I will fix everything. Now I really don't think that feature will come out in v4.0.0.

…

On Thu, 15 Sept 2022, 02:16 Matt Wang, ***@***.***> wrote: Hey @simonebortolin <https://github.com/simonebortolin>, just wanted to check - were you hoping to merge this in for the upcoming release? If so, I think there's a merge conflict / CI issues, but after those are resolved happy to give a review! — Reply to this email directly, view it on GitHub <#945 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGMZW4DXFYODKLAA2DQTPXDV6JTHHANCNFSM6AAAAAAQBRPPSM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mattxwang · 2022-09-15T18:43:53Z

No worries, sounds good!

simonebortolin · 2022-09-15T18:53:57Z

I've already sketched out the fix anyway, it just needs to be finished and committed Simone

…

On Thu, 15 Sept 2022, 20:44 Matt Wang, ***@***.***> wrote: No worries, sounds good! — Reply to this email directly, view it on GitHub <#945 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGMZW4HLE5HXCR2SSFINAWLV6NU7HANCNFSM6AAAAAAQBRPPSM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

simonebortolin · 2022-09-16T21:34:31Z

Hello @mattxwang everything should now be complete

I should have synchronised the repo, I rebased a few commits because there were so many to manage in one PR

And I launched npm test, fixed everything I modified, but I get errors on code I haven't touched.

If there are other changes I will make them next week.

N.B.

to fix this > maybe I could use :first-letter, but I don't know if it's worth it since that listing doesn't work anyway

simonebortolin · 2022-09-24T08:21:08Z

@mattxwang can you try to launch the workflow? these days I am free and will fix any problems

simonebortolin · 2022-09-25T07:36:23Z

@mattxwang can you try again

mattxwang

Hey @simonebortolin, a couple of broad stroke notes:

if you're adding a configuration option, we should add some documentation explaining how users could use it. Can you add some writing to the configuration page to explain how to use this feature?
what's the driving use-case for customizing the timeout time? I imagine it would be relatively standardized?

the icons come from bootstrap icons, licensed by MIT, I don't think anything else needs to be added

this is not true - if the icons are MIT licensed, we're obligated to redistribute a copy of the license (and, additionally, we should credit them)
there are some areas of the site where the new styling you've added changes the spacing in a code block. For example, on the code page on main:

The same page on the deploy preview:

Note the shifted backticks/js!

Appreciate your work on this PR. I think with a bit of tidying, we'll be able to merge in no time!

simonebortolin · 2022-09-26T14:26:41Z

@mattxwang

if you're adding a configuration option, we should add some documentation explaining how users could use it. Can you add some writing to the configuration page to explain how to use this feature?

done

what's the driving use-case for customizing the timeout time? I imagine it would be relatively standardized?

none, but one more option is always better than less

the icons come from bootstrap icons, licensed by MIT, I don't think anything else needs to be added

this is not true - if the icons are MIT licensed, we're obligated to redistribute a copy of the license (and, additionally, we should credit them)

yes, but JTD is also via MIT licence, so the licence copy is already there. I don't think anything more needs to be said, maybe I'm wrong.

there are some areas of the site where the new styling you've added changes the spacing in a code block. For example, on the code page on main:

done

mattxwang

Sorry, have been in and out - personal stuff.

none, but one more option is always better than less

Hm, I slightly disagree, since this is extra stuff we need to document and maintain. If there's no motivating use-case, can we not include this for now? It's much easier for us to add features later on, once people request it.

yes, but JTD is also via MIT licence, so the licence copy is already there. I don't think anything more needs to be said, maybe I'm wrong.

Ah, not exactly. We need to include a different license copy for each different project we use; this is because each library has its own copyright notice attached to the MIT license. For example, this repo's reads:

Copyright (c) 2016 Patrick Marsceill

Which is the original creator of the theme. In contrast, the Bootstrap Icon MIT License reads

Copyright (c) 2019-2021 The Bootstrap Authors

Light-touch changes left, but if we address those two points I'd be happy to merge this in! Apologies for how long this has taken, usually my iteration loop is tighter.

docs/ui-components/code.md

olgarithms

Thanks @simonebortolin for implementing this feature. As a user of the theme I am very interested, so I took a look and left a couple of comments in regards to naming:

assets/js/just-the-docs.js

_config.yml

simonebortolin · 2022-10-23T12:02:19Z

Thanks @simonebortolin for implementing this feature. As a user of the theme I am very interested, so I took a look and left a couple of comments in regards to naming:

yes you are right, but @mattxwang told me to remove it

@mattxwang sorry I've been full too, next week I'll make the changes

mattxwang · 2022-10-23T18:05:11Z

No worries, take your time @simonebortolin!

simonebortolin · 2022-12-24T12:20:27Z

@mattxwang OK I'm there

I added the image licence following the style of https://github.com/just-the-docs/just-the-docs/blob/main/_includes/icons/external_link.html
I have apply your and @olgarithms suggestion
I rebased everything with the last commit in main branch

I wish happy holidays to all

I am so sorry for my absence

- add icon, css, and js for the copy button on each 'code' enviroment - add yml/liquid option for disable copy button - add relative doc

_config.yml

mattxwang

Thanks again @simonebortolin for taking this up, and don't apologize at all for taking a while - this is free open-source, and you're volunteering your time!

Overall I think this is pretty good to merge; I've left some minor nits in code review. I've also noticed on mobile that the clipboard gets clipped; this is me on Safari on iOS:

To be frank, I'm actually fine with merging this as-is with the mobile bug (and fixing it in a follow-up), since I think it's not breaking the feature + it's uncommon for users to copy code from a mobile device. If you can address the suggested comments, we can merge this in for the last RC of this calendar year!

assets/js/just-the-docs.js

_sass/code.scss

Co-authored-by: Matt Wang <matt@matthewwang.me>

mattxwang

Leaving a green check since this is technically in a mergeable state, but @simonebortolin it would be great if you could also prune the CSS properties and/or add some docs.

I'll merge this in before releasing our RC & resolve the merge conflict with #1058 (should be trivial).

simonebortolin · 2022-12-26T23:10:15Z

@mattxwang OK then

I have put some documentation, if you do not find it sufficient I will fix all comments according to the new logic
It is very difficult to reduce the css code, it is all written in the first paragraph because a massive edit was necessary, as the way the css code was structured before made it impossible to add the button.
for refactor: modularize site components #1058 do I have to do it?

simonebortolin · 2022-12-27T00:35:20Z

@mattxwang I have added comments explaining the logic of how code.scss works

bitterly unable to reduce the number, I gave the best of my knowledge to get this PR right

mattxwang · 2022-12-27T00:43:50Z

No worries, thanks for taking this on!

do I have to do it?

Nope, I'll handle it since I'm authoring the other PR!

mattxwang · 2022-12-27T00:46:03Z

Thanks for taking this on! Much appreciated @simonebortolin, and congrats on your first contribution!

simonebortolin · 2022-12-27T00:51:02Z

No worries, thanks for taking this on!

it was beautiful!

Thanks for taking this on! Much appreciated @simonebortolin, and congrats on your first contribution!

Thanks! come on that the second PR is already in the working

simonebortolin · 2022-12-30T19:09:27Z

assets/js/just-the-docs.js

+
+    copyButton.addEventListener('click', function () {
+      if(timeout === null) {
+        var code = codeBlock.querySelector('code').innerText.trim();


@pdmosses i think the problem is here #1096

now that I see there is also another problem:
the querySelector('code') in the case of copy button with line number copy also the number of line.

@simonebortolin

This PR fixes three bugs: # first bug When revising my last PR #1086 I realised a slight bug in the code-copy PR #945 , my change to the css ignored a case. This PR is a hotfix and Before PR #945: ![image](https://user-images.githubusercontent.com/26844016/209864912-2fe8e5a9-f21e-40c7-aa0d-65050196f4ee.png) ![image](https://user-images.githubusercontent.com/26844016/209864950-c315cef1-36ee-4356-91b2-db159cf3806f.png) After PR #945: ![image](https://user-images.githubusercontent.com/26844016/209864524-70a8b095-056a-464b-9ff7-fd31397492ba.png) ![image](https://user-images.githubusercontent.com/26844016/209864558-9fd7a5d3-a965-4aa4-af62-a56846e331b3.png) Fix: ![image](https://user-images.githubusercontent.com/26844016/209865514-a9921096-b852-4402-8272-b76908851ad6.png) ![image](https://user-images.githubusercontent.com/26844016/209865550-d7842507-74fc-4f21-b407-9b8917df1fd8.png) # second bug > @simonebortolin @mattxwang I'm trying to write some regression tests for this feature. > > If I use GitHub's copy button to copy the following plain text, it preserves all the spaces: > > ``` > 1 leading space > 2 leading spaces and 2 trailing spaces > 3 internal spaces > 4 trailing spaces > ``` > > Using the new copy button with the same text in this PR branch of JTD gives this: > > ``` > 1 leading space > 2 leading spaces and 2 trailing spaces > 3 internal spaces > 4 trailing spaces > ``` > > It appears that the leading space from line 1 has been removed, and inserted on all the other lines. Moreover, the 4 trailing spaces have been removed. > > BTW, #924 didn't give a precise requirements spec, but mentioned the Microsoft docs UI; @mattxwang mentioned also the GitHub UI. It would be helpful to add a functional spec of what the JTD copy button is supposed to do, as a basis for regression tests. > > I'm not aiming at a rigorous test for the UI. Personally (using Safari at the default mag) I find the clipboard icon too small: it just looks like a box, and I can hardly see that there is a clip at the top. But I don't have a suggestion for a better icon. # third bug When I re-read the code after the second bug, I noticed a bug that it does not always select the text field to be copied correctly (in case there are also line numbers) is copied: ``` 1 2 3 4 # Ruby code with syntax highlighting and fixed line numbers using Liquid GitHubPages::Dependencies.gems.each do |gem, version| s.add_dependency(gem, "= #{version}") end ``` instead of ``` # Ruby code with syntax highlighting and fixed line numbers using Liquid GitHubPages::Dependencies.gems.each do |gem, version| s.add_dependency(gem, "= #{version}") end ``` Co-authored-by: Matt Wang <matt@matthewwang.me>

simonebortolin force-pushed the copy-button branch from 19a9f33 to f6a96f7 Compare September 3, 2022 19:41

simonebortolin closed this Sep 16, 2022

simonebortolin force-pushed the copy-button branch from 7cca09b to b996426 Compare September 16, 2022 21:17

simonebortolin reopened this Sep 16, 2022

mattxwang changed the title ~~Add copy button for copying in the code snippets~~ Add button to copy code in code snippets Sep 26, 2022

mattxwang changed the title ~~Add button to copy code in code snippets~~ Add copy code button to code snippets Sep 26, 2022

mattxwang requested changes Sep 26, 2022

View reviewed changes

simonebortolin requested a review from mattxwang September 26, 2022 14:26

mattxwang requested changes Oct 10, 2022

View reviewed changes

docs/ui-components/code.md Outdated Show resolved Hide resolved

docs/ui-components/code.md Outdated Show resolved Hide resolved

olgarithms reviewed Oct 23, 2022

View reviewed changes

assets/js/just-the-docs.js Outdated Show resolved Hide resolved

_config.yml Outdated Show resolved Hide resolved

simonebortolin force-pushed the copy-button branch from 70b7a3f to cf4cb08 Compare December 24, 2022 12:17

Add copy button:

b51e943

- add icon, css, and js for the copy button on each 'code' enviroment - add yml/liquid option for disable copy button - add relative doc

simonebortolin force-pushed the copy-button branch from cf4cb08 to b51e943 Compare December 24, 2022 12:31

simonebortolin added a commit to simonebortolin/just-the-docs that referenced this pull request Dec 24, 2022

Add copy button (just-the-docs#945)

60134f7

- add icon, css, and js for the copy button on each 'code' enviroment - add yml/liquid option for disable copy button - add relative doc

max06 reviewed Dec 24, 2022

View reviewed changes

_config.yml Outdated Show resolved Hide resolved

Apply code review by @max06

e4db004

simonebortolin mentioned this pull request Dec 26, 2022

Implement toggle color scheme, automatic theme mode #1086

Open

mattxwang reviewed Dec 26, 2022

View reviewed changes

assets/js/just-the-docs.js Outdated Show resolved Hide resolved

_sass/code.scss Show resolved Hide resolved

_sass/code.scss Show resolved Hide resolved

_sass/code.scss Outdated Show resolved Hide resolved

Apply suggestions from code review

a7a2ccd

Co-authored-by: Matt Wang <matt@matthewwang.me>

mattxwang approved these changes Dec 26, 2022

View reviewed changes

add comment and fix on safari bug

b7aa2c2

simonebortolin added 3 commits December 27, 2022 00:10

Merge branch 'main' into copy-button

bab8990

fix npm test

7b5fdec

add comment

4919743

mattxwang merged commit ce3f34b into just-the-docs:main Dec 27, 2022

simonebortolin deleted the copy-button branch December 27, 2022 00:49

mattxwang mentioned this pull request Dec 27, 2022

Add a button for copying in the code snippets #924

Closed

simonebortolin mentioned this pull request Dec 28, 2022

Hot fix on copy code button #1096

Merged

simonebortolin commented Dec 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add copy code button to code snippets #945

Add copy code button to code snippets #945

simonebortolin commented Aug 31, 2022

mattxwang commented Sep 2, 2022

simonebortolin commented Sep 2, 2022 via email

simonebortolin commented Sep 3, 2022

simonebortolin commented Sep 3, 2022 •

edited

Loading

simonebortolin commented Sep 5, 2022

mattxwang commented Sep 6, 2022 •

edited

Loading

mattxwang commented Sep 15, 2022

simonebortolin commented Sep 15, 2022 via email

mattxwang commented Sep 15, 2022

simonebortolin commented Sep 15, 2022 via email

simonebortolin commented Sep 16, 2022 •

edited

Loading

simonebortolin commented Sep 24, 2022

simonebortolin commented Sep 25, 2022

mattxwang left a comment

simonebortolin commented Sep 26, 2022

mattxwang left a comment

olgarithms left a comment

simonebortolin commented Oct 23, 2022

mattxwang commented Oct 23, 2022

simonebortolin commented Dec 24, 2022 •

edited

Loading

mattxwang left a comment

mattxwang left a comment

simonebortolin commented Dec 26, 2022

simonebortolin commented Dec 27, 2022

mattxwang commented Dec 27, 2022

mattxwang commented Dec 27, 2022

simonebortolin commented Dec 27, 2022 •

edited

Loading

simonebortolin Dec 30, 2022 •

edited

Loading

Add copy code button to code snippets #945

Add copy code button to code snippets #945

Conversation

simonebortolin commented Aug 31, 2022

mattxwang commented Sep 2, 2022

simonebortolin commented Sep 2, 2022 via email

simonebortolin commented Sep 3, 2022

simonebortolin commented Sep 3, 2022 • edited Loading

simonebortolin commented Sep 5, 2022

mattxwang commented Sep 6, 2022 • edited Loading

mattxwang commented Sep 15, 2022

simonebortolin commented Sep 15, 2022 via email

mattxwang commented Sep 15, 2022

simonebortolin commented Sep 15, 2022 via email

simonebortolin commented Sep 16, 2022 • edited Loading

simonebortolin commented Sep 24, 2022

simonebortolin commented Sep 25, 2022

mattxwang left a comment

Choose a reason for hiding this comment

simonebortolin commented Sep 26, 2022

mattxwang left a comment

Choose a reason for hiding this comment

olgarithms left a comment

Choose a reason for hiding this comment

simonebortolin commented Oct 23, 2022

mattxwang commented Oct 23, 2022

simonebortolin commented Dec 24, 2022 • edited Loading

mattxwang left a comment

Choose a reason for hiding this comment

mattxwang left a comment

Choose a reason for hiding this comment

simonebortolin commented Dec 26, 2022

simonebortolin commented Dec 27, 2022

mattxwang commented Dec 27, 2022

mattxwang commented Dec 27, 2022

simonebortolin commented Dec 27, 2022 • edited Loading

simonebortolin Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

simonebortolin commented Sep 3, 2022 •

edited

Loading

mattxwang commented Sep 6, 2022 •

edited

Loading

simonebortolin commented Sep 16, 2022 •

edited

Loading

simonebortolin commented Dec 24, 2022 •

edited

Loading

simonebortolin commented Dec 27, 2022 •

edited

Loading

simonebortolin Dec 30, 2022 •

edited

Loading