Add copy button for link generator

[jrdnbradford]

I added a copy button for the generated link field to make it easier for users. I don't consider myself much of a web developer, but maybe it's not totally wrong. 😃

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[yuvipanda]

Yay, thank you for your PR, @jrdnbradford :) I think this is useful functionality!

I've asked @batpad to review this PR, to increase the amount of review capacity in the Jupyter ecosystem. He's taking a look and should respond soon.

[batpad]

Hey @jrdnbradford thanks much for the contribution! It overall looks really good and seems super useful!

Couple minor things:

• There seem to be minor CSS issues - the border for the JupyterHub URL input field seems to go behind the Generated Link input and looks a bit odd? Perhaps we can just increase the margin a little bit between the Generated Link input and the JupyterHub URL input? (I see this on Chrome on a Mac)

• In my (limited) tests, one did not need to select the text to have the "copy" working. Although, I can see it might be nice to do the text selection, especially as an indicator to the user that "something happened" when they clicked Copy. If we want to keep the text selection, my recommendation to make things a bit cleaner would be instead of:

  copyText.setSelectionRange(0, 99999);

To do something like:

copyText.setSelectionRange(copyText.value.length)

This should also select the whole text without needing to hard-code this 99999 magic number :-)

Getting copying to work consistently across browsers has always been a bit of a pain, but it seems to me this clipboard API is pretty stable across browsers now and does not need additional hacks for browser compatibility? https://caniuse.com/mdn-api_clipboard_writetext - it would still be good to test across a few browsers - I've so far tested in Chrome and Firefox and it looks good to me!

@jrdnbradford would you be able to look at the couple of minor things above? Once those are fixed, I'd be 👍 on merging.

[jrdnbradford]

Awesome, thanks for this feedback, @batpad.

Here's what I did:

• updated it to use focus() instead of select(). I think this gives the cleaner look you were hoping for in the UI (and the code) and still lets the user know something happened
• add a small margin to the input selection that separates the fields
• tested on Firefox, Chrome, and Edge on my Linux machine

Let me know if you'd like to see any more changes. 🎉

[batpad]

@jrdnbradford thanks for the quick fixes!

Seems like we might be going down a bit of a CSS rabbit-hole where you bloop one thing and another thing blops:

Now the branch field is no longer properly aligned with the Github Repository URL field. Unfortunately, CSS is not my strong suit to have concrete suggestions on fixes :(. From just looking at things a bit, I see you've added some custom styling using flex-box for the input-group class you added. While I think I agree this is likely a better approach to styling, do you think it might be easier to be more consistent with the rest of the form and put things in col- classes - similar to how the Github Repository URL and Branch bits are styled?

This CSS stuff can be a bit harrowing - let me know if you're able to look / fix, else I can look at spending some time on it / pull in someone who's a bit more experienced with CSS.

[jrdnbradford]

I should have caught that before, sorry for that. I removed the flex-box setup and things don't seem to get bumped around anymore. Good catch! I'm not the best with CSS either.

[batpad]

This looks good to me!

@yuvipanda how do you feel about merging this?

(@jrdnbradford apologies for the delay here - I had a few days of travel this week :) )

[batpad]

updated it to use focus() instead of select(). I think this gives the cleaner look you were hoping for in the UI (and the code) and still lets the user know something happened

Just thinking about this a bit more -- @jrdnbradford how do you feel about moving back to the select approach, but just replacing the 99999 magic number with something like copyText.value.length ? I think selecting the text there is nice, allows the user to Ctrl-C in case for some reason the copying to clipboard doesn't work (I'm guessing there's edge cases where users have privacy controls and / or browser extensions that prevent websites from copying onto clipboard), and I think the selection is a bit nicer than focus to indicate "something happened".

This is really not a hard opinion at all, but if that resonates with you @jrdnbradford, I'd welcome that change. If not, I think the current state is fine as well!

[jrdnbradford]

Agreed 💯 , and done!

[yuvipanda]

yay, thank you very much for your review, @batpad!

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[yuvipanda]

And thank you so much for your contributions, @jrdnbradford!!!