Handle copy clipboard disabled #409
Conversation
frontend/src/upload.js
Outdated
| @@ -18,6 +18,21 @@ if (storage.has('referrer')) { | |||
| window.referrer = 'external'; | |||
| } | |||
|
|
|||
| const allowedCopy = () => { | |||
| const support = !!document.queryCommandSupported | |||
| return support ? document.queryCommandSupported('copy') : false | |||
There was a problem hiding this comment.
Oups I should add semicolons here
frontend/src/upload.js
Outdated
| $('#link').attr('disabled', false); | ||
| $copyBtn.attr('data-l10n-id', 'copyUrlFormButton'); | ||
| }, 3000); | ||
| const copyResult = copyToClipboard($('#link').attr('value')); |
There was a problem hiding this comment.
copyResult not necessary
| @@ -79,23 +94,21 @@ $(document).ready(function() { | |||
| sendEvent('sender', 'copied', { | |||
There was a problem hiding this comment.
can we move the GA event inside of the if (allowedCopy()) { block?
frontend/src/upload.js
Outdated
| $('#link').attr('disabled', false); | ||
| $copyBtn.attr('data-l10n-id', 'copyUrlFormButton'); | ||
| }, 3000); | ||
| const copyResult = copyToClipboard($('#link').attr('value')); |
There was a problem hiding this comment.
copyToClipboard($('#link').attr('value')) should also move inside the if block
There was a problem hiding this comment.
Sure that's what I'll do 👍
| @@ -504,6 +509,7 @@ tbody { | |||
| background: #05a700; | |||
| border: 1px solid #05a700; | |||
| cursor: auto; | |||
| opacity: 0.3; | |||
There was a problem hiding this comment.
Just a question of opinion but for me it's a better visual feedbacks for the users
There was a problem hiding this comment.
oh sorry, I read it wrong. I thought it was within .popup. this is good
frontend/src/upload.js
Outdated
| @@ -345,6 +358,9 @@ $(document).ready(function() { | |||
| class: 'icon-copy', | |||
| 'data-l10n-id': 'copyUrlHover' | |||
| }); | |||
| if (!allowedCopy()) { | |||
| $copyIcon.addClass('disabled'); | |||
There was a problem hiding this comment.
since 'disabled' is really common, can we add a 'disabled' attribute instead? Then check the css with :disabled
we already have a rule for #copy-btn:disabled that can be reused.
There was a problem hiding this comment.
I'll push changes you asked, but for this change #copy-btn:disabled rule cannot be reused because .icon-copy is an image but I used disabled attribute instead of .disabled
|
thanks so much @Johann-S for taking this one :) |
|
You're welcome @ericawright a pleasure to help Mozilla 👍 Just a question out of the scope of this PR, are you open for some refactorisation of your front JavaScript because I noticed some bad practices (events alias, duplicate selectors...) ? |
|
@Johann-S of course, you can open a bug explaining each of the changes needed, or you can just go ahead and open a PR. |
|
Oh ok so should I revert this change : https://github.com/Johann-S/send/blob/patch-1/frontend/src/upload.js#L113-L119 ? |
|
It's a good change, but it's unrelated to the current changes. if it's just that then it's fine, might as well leave it in. but if there are other unrelated changes then a separate PR would make it clearer to review |
|
No it's the only one 👍 My last concern about this PR, it's when we want to copy an old uploaded file link and if copy/paste command isn't available currently it's isn't possible 🤔 That's why I shown this image : Should I change the icon by an input with the URL ? Maybe If we can have a QRCode for each link it will be a good solution to show a Popover with the QRCode (asked feature here : #423) |
|
I see. I'd be willing to merge this without that enhancement, but yes, it would be more convenient to show the URL there. |
|
I think if it's ok for you, you can merge this PR, and I'll make a new PR for this issue when I'll find time to work on that 👍 |
Should handle disabled copy to clipboard.
But I have to find a way for something better than that :
Fixes #108