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

[VDG] clear clipboard on leaving dialog #10558

Merged
merged 3 commits into from May 4, 2023

Conversation

soosr
Copy link
Collaborator

@soosr soosr commented Apr 25, 2023

fixes #10523

@turbolay
Copy link
Collaborator

Just a question: No need to inform the user of this behavior?

yahiheb
yahiheb previously approved these changes Apr 26, 2023
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@yahiheb
Copy link
Collaborator

yahiheb commented Apr 26, 2023

Just a question: No need to inform the user of this behavior?

Good question but how would you do that?

@soosr
Copy link
Collaborator Author

soosr commented Apr 26, 2023

Just a question: No need to inform the user of this behavior?

This question came up before, and it was said we don't have to expose implementation details in front of the user.

Good question but how would you do that?

I am personally not happy with the context menu. I mentioned several times that people don't know that they exist. And having a feature only available via the context menu is a bad approach.

I proposed a solution that is standard in Wasabi. When the cursor enters the recovery words area, in the bottom right a copy button could appear. And the tooltip could say Temporary copy the words. They will disappear from the clipboard once you left this page.

@yahiheb
Copy link
Collaborator

yahiheb commented Apr 26, 2023

I am personally not happy with the context menu. I mentioned several times that people don't know that they exist. And having a feature only available via the context menu is a bad approach.

I proposed a solution that is standard in Wasabi. When the cursor enters the recovery words area, in the bottom right a copy button could appear. And the tooltip could say Temporary copy the words. They will disappear from the clipboard once you left this page.

If this feature was needed and users should know about it then I agree with your solution, it is better than the context menu. But my opinion is just to remove this feature/issue completely.

Currently we added it but we made it as hidden as possible which kind of indicates that we don't want users to know about it, which raises the question why did we add it then?

This reverts commit d17911e.
@Pule08
Copy link
Collaborator

Pule08 commented May 2, 2023

@nopara73 what do you think about @soosr solution? Is it something which worth to finish?

@nopara73
Copy link
Contributor

nopara73 commented May 3, 2023

It seems this PR removes the 30 seconds timeout. Why?

@soosr
Copy link
Collaborator Author

soosr commented May 3, 2023

It seems this PR removes the 30 seconds timeout. Why?

The clipboard will be cleared out when the user leaves the dialog. IMO this approach makes more sense and it is more consistent as #10523 suggests it.

@nopara73 what do you think about @soosr solution? Is it something which worth to finish?

This question was for this #10558 (comment) comment.
What do you think about the proposed solution:

When the cursor enters the recovery words area, in the bottom right a copy button could appear. And the tooltip could say Temporary copy the words. They will disappear from the clipboard once you left this page.

@nopara73
Copy link
Contributor

nopara73 commented May 4, 2023

  1. Makes sense. I didn't think of it that way.
  2. No.

Copy link
Contributor

@nopara73 nopara73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK. Code LGTM. Didn't test.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@soosr soosr merged commit b6bc442 into zkSNACKs:master May 4, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Copyable Recovery Words
6 participants