-
Notifications
You must be signed in to change notification settings - Fork 479
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
Add copy to clipboard button for code snippets #1454
Add copy to clipboard button for code snippets #1454
Conversation
d214bbf
to
f28bdb3
Compare
Nice! Is it possible to get actual newlines instead of the character
|
Yeah, I'm trying to figure out a way to do it. Is there anything we can do from Julia side? The other option would be to handle via javascript. |
If you generate <button class="copy-button button fas fa-copy" data-clipboard-text="1 + 1
2 + 2"></button> instead of <button class="copy-button button fas fa-copy" data-clipboard-text="1 + 1\n2 + 2"></button> it seems to work. |
It would also be nice to strip
|
For the newlines, I think we should modify the HTML generation in
As long as we render it as a single |
src/Writers/HTMLWriter.jl
Outdated
language = Utilities.codelang(c.language) | ||
language = isempty(language) ? "none" : language | ||
pre(code[".language-$(language)"](c.code)) | ||
code_block = [ | ||
button[".copy-button .button .fas .fa-copy", Symbol("data-clipboard-text")=>c.code], |
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.
Might be nice to have a title
attribute on each button as well that would probably say "Copy to clipboard".
Valid point, but it kind of defeats the purpose of having the button if the result is not pasteable into a Julia REPL? But perhaps it would be nice to render as two blocks instead, the |
I think that's worth considering (#1455), but let's leave it for a separate PR. |
It would be nice to also show |
694c73e
to
6cae6cd
Compare
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.
Just a bunch of bikeshedding thoughts:
-
I think an arrow-pointing-to-clipboard icon (e.g. like they use on the clipboard.js website) would be better, but I couldn't actually find one in FontAwesome, so we can probably stick with the current one.
-
We could make the button hide itself if you're not hovering over the
<pre>
box (like we do with the "Source" links on docstrings), so it would not distract when not necessary. -
How hard do you think would it be to make the "Copied" text appear next to the button (e.g. in a bubble, like on the clipboard.js site), as opposed to replacing the icon inside the button? It's just the button changing size does not look the prettiest in my opinion.
@@ -0,0 +1,3 @@ | |||
.copy-button { | |||
float: right; |
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 should make sure that it's the same distance from top and right side of the box:
float: right; | |
float: right; | |
top: -0.2rem; |
Oh, one more thing, would you mind adding an entry into the CHANGELOG.md file as well about this PR? |
src/Utilities/DOM.jl
Outdated
end | ||
if occursin("\n", text) | ||
replace(text, "\n"=>" ") |
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 should go into the if
/for
above. Currently it's actually breaking the escaping (the top if
never returns from the function).
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.
Yeah, I realised that. The problem here though is that if I replace it there, the code block doesn't render properly. I believe highlight.js needs those "/n"
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.
Right. Should be okay if we only escape \n
-s in attributes? I think we could implement that by adding a keyword argument to escapehtml
and toggling that wherever we're escaping the attribute contents?
@mortenpi Have a look at this once you're free, I've tried to address as many comments as I could. Do let me know what you think about this :) |
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.
d0fd30c
to
5d4ef78
Compare
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.
@abhishalya I'm really sorry for not reviewing earlier -- I completely missed that you updated the PR.
Besides the escaping thing, the only other issue I can still see is the "Copied" message creating scrollbars, instead of being of above everything. I tried to find a way around it, but I could quite figure it out. It feels like the solution in this post might be applicable.
@@ -281,7 +281,7 @@ When no escaping is needed then the same object is returned, otherwise a new | |||
string is constructed with the characters escaped. The returned object should | |||
always be treated as an immutable copy and compared using `==` rather than `===`. | |||
""" | |||
function escapehtml(text::AbstractString) | |||
function escapehtml(text::AbstractString; escape_newlines=false) | |||
if occursin(r"[<>&'\"]", text) |
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 we need to do something like this
if occursin(r"[<>&'\"]", text) | |
if occursin(r"[<>&'\"\\n]", text) |
because it currently doesn't escape newlines if there are not other special characters:
julia> Documenter.Utilities.DOM.escapehtml("xyz\nAAA", escape_newlines=true)
"xyz\nAAA"
And I think we could avoid the separate if
, and just add another char === '\n'
into the for loop below?
Preview:
Closes #1055