Add copy-to-clipboard element for upload/download lists.#1099
Add copy-to-clipboard element for upload/download lists.#1099ArneBab wants to merge 1 commit intohyphanet:nextfrom
Conversation
bertm
left a comment
bertm
left a comment
There was a problem hiding this comment.
Feature-wise this is nice, the implementation could use some additional care.
| if (core.getNode().isFProxyJavascriptEnabled()) { | ||
| form.addChild("script", new String[] {"type", "src"}, new String[] {"text/javascript", "/static/js/clipboard.js"}); | ||
| } |
There was a problem hiding this comment.
Loading it here causes the script to be included more than once when there are multiple tables on the page (like when there are both completed and running downloads), rendering duplicate buttons for each key.
(if we cannot prevent the script from loading twice for some reason, the script should guard for this condition)
| // @license magnet:?xt=urn:btih:1f739d935676111cfff4b4693e3816e664797050&dn=gpl-3.0.txt GPL-v3-or-Later | ||
| function addCopyKeyFieldToFinishedTransfers() { | ||
| if (navigator && navigator.clipboard) { | ||
| var matching = document.getElementsByClassName("finished_key_link"); |
There was a problem hiding this comment.
Can we make this copy-to-clipboard functionality agnostic of the context in which it is used?
One could search for class="copy-to-clipboard" and use data-copy-text="{link or whatever text to copy}" and data-copy-title="{tooltip from i18n}".
| toClipboard.classList.add("copy-to-clipboard-element"); | ||
| toClipboard.setAttribute("title", "Copy to Clipboard"); | ||
| toClipboard.onclick=() => navigator.clipboard.writeText(key); | ||
| matchingElement.parentElement.appendChild(toClipboard); |
There was a problem hiding this comment.
This could directly use after() on matchingElement instead of traversing to the parent element.
| @@ -0,0 +1,19 @@ | |||
| // @license magnet:?xt=urn:btih:1f739d935676111cfff4b4693e3816e664797050&dn=gpl-3.0.txt GPL-v3-or-Later | |||
| function addCopyKeyFieldToFinishedTransfers() { | |||
| if (navigator && navigator.clipboard) { | |||
There was a problem hiding this comment.
The Window navigator property has been supported by browsers for over 10 years (including IE6 and the initial releases of Chrome, Firefox, and Edge).
There really is no need to check for that property to exist 😃
| var toClipboard = document.createElement("span"); | ||
| toClipboard.textContent = " ⎘"; |
There was a problem hiding this comment.
Best to not use whitespace characters for positioning. A better approach would be to set a margin-inline-start in CSS. While styling, also consider user-select: none to prevent the text from being selected and cursor: pointer to indicate that this is an action.
| var key = matchingElement.dataset.key; | ||
| if (key) { | ||
| var toClipboard = document.createElement("span"); | ||
| toClipboard.textContent = " ⎘"; |
There was a problem hiding this comment.
As for the character (⎘), this is not something that I would immediately recognize.
The emoji glyph for clipboard (📋) might be more visually recognizable (depending on OS support). What's your take?
There was a problem hiding this comment.
The character ⎘ is the only one that’s universally supported. I don’t actually see 📋 on my system (copied it as empty block).
There was a problem hiding this comment.
Mmh. In that case we might be better off rendering an SVG, as the character may be too font-dependent.
That could be something like this (quick sketch based on distant memories of similar functionality):
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" style="height: 1em; width: 1em; margin-inline-start: .5ex; vertical-align: middle;">
<path stroke="currentColor" stroke-width="2" d="M 2 16 v -12 a 2 2 0 0 1 2 -2 h 12 M 9 7 h 10 a 2 2 0 0 1 2 2 v 10 a 2 2 0 0 1 -2 2 h -10 a 2 2 0 0 1 -2 -2 v -10 a 2 2 0 0 1 2 -2" fill="none"></path>
</svg>
There was a problem hiding this comment.
Is this duplication necessary? If all is well .. all themes include src/freenet/clients/http/staticfiles/color.css anyways.
There was a problem hiding this comment.
Some themes don’t include color.css, because that actually causes latency of displaying toadlets to increase (despite being a local service, our static file serving is pretty slow).
That can totally be worked around by putting the translated texts into an invisbile |
3 participants
The title text currently cannot be internationalized, because I wanted to keep the initialization out of the Toadlets.