Strip $ symbol from code block text #1110
Conversation
jasonhr13
changed the title
jasonhr13
changed the title
There was a problem hiding this comment.
Thank you for this PR!
To the current maintainers
It has been some time since I have been involved in the project, I hope it is ok if I perform a first review of this PR?
@jasonhr13
I like the idea in theory, but I have a little concern about the implementation.
First of all, I would be really conservative about the code blocks this is applied on. I would only perform this dollar stripping when the code block is explicitly marked as bash or shell.
Because there certainly exists a language or format (logs, configs, ...) that will be negatively impacted if we apply this globally.
Secondly, I wonder if there is any possibility of encountering a valid (meaningful) line of bash that begins with $ (dollar + space). Because if there is, I would find it more concerning to silently strip parts of valid commands. Otherwise I would be inclined to accept this PR.
Finally, the same convention exists where you prefix # for commands run as root. Do we want to handle this too?
| @@ -6,12 +6,19 @@ window.onunload = function () { }; | |||
| // Global variable, shared between modules | |||
| function playpen_text(playpen) { | |||
| let code_block = playpen.querySelector("code"); | |||
| let regex = /^\$(?: )/; | |||
There was a problem hiding this comment.
I would suggest a more explicit variable name for this regex.
| let regex = /^\$(?: )/; | |
| let regex_leading_dollar_space = /^\$(?: )/; |
|
|
||
| if (window.ace && code_block.classList.contains("editable")) { | ||
| let editor = window.ace.edit(code_block); | ||
| return editor.getValue(); | ||
| } else { | ||
| return code_block.textContent; | ||
| // It is common for code blocks to contain $ in the text. If they do, clipboard will copy that symbol and the ensuing paste into terminal will fail |
There was a problem hiding this comment.
This comment is great because it explains the motivation. I would specify that the dollar sign convention is common for bash / shell specifically, not all code blocks. AndI would expand this comment to mention briefly how this is solved (stripping dollar signs)
|
|
||
| if (window.ace && code_block.classList.contains("editable")) { | ||
| let editor = window.ace.edit(code_block); | ||
| return editor.getValue(); | ||
| } else { | ||
| return code_block.textContent; | ||
| // It is common for code blocks to contain $ in the text. If they do, clipboard will copy that symbol and the ensuing paste into terminal will fail | ||
| if (regex.test(code_block.textContent)) { |
There was a problem hiding this comment.
Is this if necessary? If we do a replace and nothing matches we are going to retrieve the input string at the output, no?
|
@azerupi I 100% agree with everything you said. My ideal scenario, and what you typically see when using something like ClipboardJS is that the text content of the code block is not copied; rather a data-clipboard attribute is applied to the clip-icon button with the proper raw data to be copied. In this scenario with how it is being generated from markdown automatically, we would need to know what that raw data is. Do we exclude This is what I did locally to be able to read the book and use the copy icon. And I know from the thousands of lines of code I have clicked to copy over the years that I have never see one copy the |
|
@azerupi You are always welcome to help! 😄 I haven't looked at this closely, but I did want to quickly mention that the |
|
@jasonhr13 in order to move forward with this, would you be willing to change the PR to:
This would be enough for me to approve the changes 🙂 |
WIP, not sure if right approach