Skip to content
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

Desktop: Fixes #4658: Added a copy button and prevented showing default context menu #4749

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

xUser5000
Copy link
Contributor

@xUser5000 xUser5000 commented Mar 28, 2021

Description

This is a quick but not long-term fix to the issue #4658.
@roman-r-m @laurent22 @CalebJohn

What has been done

  • Added a copy button next to the ID field.
  • Prevented showing the default context menu when right-clicking on the ID value.

Screenshot

Screenshot

Things to notice

const { clipboard } = require('electron');
.
.
.

clipboard.writeText(value);

This is by no means the best way to implement the functionality and there should be some sort of abstraction that hides away the details of electron clipboard object. I very much encourage people who have more knowledge to modify this. However, in the meantime, this is the quickest way to do it.

Possible improvements

  • Add a tooltip to the copy button for a better user experience.
  • Add a small dialog that indicates the text has been copied successfully.

@laurent22
Copy link
Owner

Well we don't do "quick fixes" in this app :) Unless maybe for critical urgent bugs, and this one is not.

That being said the button you've added is useful so let's keep that. Please revert the changes to the text field (disabling the context menu) as it's not necessary. Now there's two way to copy the ID - Ctrl/Cmd+C (when it works) and your button, which is more than enough.

@xUser5000
Copy link
Contributor Author

@laurent22
I did as you just said and reverted the last commit. Is it ready to be merged now ?

@laurent22
Copy link
Owner

Looks good now, thank you.

@laurent22 laurent22 merged commit c044017 into laurent22:dev Mar 29, 2021
@tessus
Copy link
Collaborator

tessus commented Apr 3, 2021

Sorry for resurrecting this thread, but I only noticed this now.

Any chance there could be some sort of feedback when clicking the icon?

Usually the icon flashes or there's some other feedback that the text was copied, but when clicking it nothing of that sort happens. Yes, the id is copied into the clipboard, but you don't know unless you check your clipboard.

Maybe this is not necessary, but it felt a bit weird.

And the context menu is still shown:

image

I can still use Cmd+C to copy the id, but the context menu has it greyed out.

@xUser5000
Copy link
Contributor Author

@tessus
I'm sorry, this is the first time I noticed your comment. I guess I should change the notifications settings a bit then 😅
Regarding your suggestion about the copy feedback, I was thinking about that at the beginning and decided it would be cool If I added that. However, I don't know why, but I forgot to work on it. So, can I start working on it now ?

On the other hand, regarding the context menu, @laurent22 said to leave it without modifications just for now. It is also possible for other developers to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants