a button to copy api token to clipboard #1345

[MontrealSergiy]

see #1345

[natacha-beck]

I have multiple comments:

• The code did not work as you expected, when you copy the API key you have space (before and after the API key you even have a \n. You should trim it.
• I do not see the necessity of having the class copiable instead you can use an ID.

I think this can do the job:

We have just generated a new API token for you (click to copy).
<p>
<div>
    <button onclick="copy_text_to_clipboard()" id="api_token"><%= @new_token %></button>
</div>
<p>

and:

function copy_text_to_clipboard() {
    navigator.clipboard.writeText($("#api_token").text().trim());
}

It gave:

Also you need to correct the date in the Copyright section.

[MontrealSergiy]

@natacha-beck do you actually observed the whitespace in copied string with your browser (safari I guess?), or trimming is 'just in case'?)

[MontrealSergiy]

I can adjust Copyrights, but fyi I think it is no longer part of cbrain development policies. Darcy had a discussion with Pierre regarding relevance of Copyright date updating, and I think he convinced everybody that copyright update is not necessary.

[natacha-beck]

I can adjust Copyrights, but fyi I think it is no longer part of cbrain development policies. Darcy had a discussion with Pierre regarding relevance of Copyright date updating, and I think he convinced everybody that copyright update is not necessary.

So do not update it, it's fine.

[natacha-beck]

@natacha-beck do you actually observed the whitespace in copied string with your browser (safari I guess?), or trimming is 'just in case'?)

I use chrome and yes I observe it by copying it, then when debugging and print in the console I was able to confirm that it copy some beginning white space and the a \n

[MontrealSergiy]

Ok then. Trimming never hurts, I do not mind to delete all the styling.

Your solution is elegant, yet ticket requested to add a new button besides token, rather than placing token into button. I do mind strongly but if you insist I have to consult with team or at the least the requester of the feature. And even in this case I suggest to add small svg copying icon besides, and, perhaps a tooltip, e.g. with

<p>
<div>
  <button onclick="copy_text_to_clipboard()" title="Click to copy the API token to the clipboard"
     id="api_token"><%= @new_token %>
    <svg width="12" height="12" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" >
      <rect x="4" y="4" width="14" height="14" rx="2" ry="2" fill="#d1d5da" stroke="#6a737d" stroke-width="1.25" />
      <rect x="7" y="7" width="14" height="14" rx="2" ry="2" fill="#d1d5da" stroke="#6a737d" stroke-width="1.25" />
    </svg>
  </button>
</div>
<p>

[MontrealSergiy]

While your token inside button suggestion has its charms, it might prevent typical 'clipboardless' copy-paste flow in unix-like OSs (selecting word with a double click and then copying it with wheel click, without need for any clipboard)

[natacha-beck]

You can discard my comment about the API button if you want, result is the same.

Your 1st solution with the trimming is good, but you can consider to remove the copiable class that is not needed.

[MontrealSergiy]

Ok, done, please, check the changes. I can rebase if you like

[prioux]

Restructure code

[MontrealSergiy]

Ok, a helper added

[prioux]

Aside from my two comments about the helper, the overall structure of this pull request is good. Isn't it much cleaner than the original design?

[prioux]

You're mutating the content of options['class'] here, so that the caller has now a different options hash after calling your helper. Use an intermediate variable or dup options

[prioux]

I'd like the caller to be able to change the text label and still get the icon. Right now if I change the label, the icon disappear.

Also the default label should be the full "Copy to clipboard", because just the word "copy" is not clear enough.

[prioux]

I'm not sure why you 'asked for my review' given nothing has changed since I last reviewed this PR and you have not addressed my comments about your helper.

[MontrealSergiy]

I might jumped the gun here. I was mainly trying to draw attention to my comment, which is mostly about label wording but could also impact other aspects. Reading it now, I realize it's a bit long and might be better discussed in person. In the meantime, I'll try to improve the helper. I'm still uncertain: (1) whether the icon or text should come first, and (2) whether a clipboard icon (or a Unicode symbol) adds more clarity than copy symbol. But I'll do my best.

[MontrealSergiy]

1. I got feeling you initially wanted to have separate helper params for the icon, distinct from the textual label, so I included one. I called it prefix, rather than icon, as it might be text or piece code, auxiliary to label itself.

If I misunderstand you, or you changed you mind, or do not like param name it is should be pretty easy to delete. one label parameter should be enough.

2. Also I am still on fence is clipboard icon better than copy icon, added both icon helpers just in the case.