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

Fix #19548 - Increase address copy to clipboard time #19948

Merged
merged 3 commits into from
Jul 12, 2023

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Jul 10, 2023

Explanation

Increases the time before clipboard gets cleared after copying a wallet address.

Manual Testing Steps

  1. From the home screen, click the address button
  2. Command-V several times to a new document until the clipboard eventually gets cleared

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@darkwing darkwing requested a review from a team as a code owner July 10, 2023 21:05
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [3603415]
Page Load Metrics (1574 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1113801505627
domContentLoaded1455178415748541
load1455178415748541
domInteractive1455178415748541
Bundle size diffs
  • background: 0 bytes
  • ui: 171 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [f983445]
Page Load Metrics (1636 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1052981434019
domContentLoaded14861937163611254
load14871937163611254
domInteractive14861937163611254
Bundle size diffs
  • background: 0 bytes
  • ui: 168 bytes
  • common: 0 bytes

@HowardBraham
Copy link
Contributor

Is the default time really only 3 seconds? I expect there's a lot more places where this will be a problem.

Also, what happens if you copy from one place, wait 55 seconds, and then copy from another? Will the clipboard clear in 5 seconds?

@NidhiKJha
Copy link
Member

I like this solution but if I copy address from account 1 then it stays copied and even I switch to account 2 the tooltip and icon is still in copied state even though the copied text is address from account 1. Functionality wise it's working fine and if I copy the address from account 2, it's updated.

Screen.Recording.2023-07-11.at.2.48.44.PM.mov

@NidhiKJha NidhiKJha added team-extension-client team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Jul 11, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [dd9c379]
Page Load Metrics (1565 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint107164128157
domContentLoaded1464179315657637
load1464179315657637
domInteractive1464179315657637
Bundle size diffs
  • background: 0 bytes
  • ui: 135 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #19948 (dd9c379) into develop (13b1f8c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop   #19948   +/-   ##
========================================
  Coverage    69.54%   69.54%           
========================================
  Files          983      983           
  Lines        37161    37161           
  Branches      9963     9963           
========================================
  Hits         25841    25841           
  Misses       11320    11320           
Impacted Files Coverage Δ
...tichain/address-copy-button/address-copy-button.js 100.00% <100.00%> (ø)

@plasmacorral
Copy link
Contributor

On the IOS mobile client there is an expectation set for the user by including the following text "stored for 1 minute" when an SRP or private key is copied. Unfortunately, there was not an effective way to enforce the clipboard purge on Android.

Should we inform the user within the UI that the clipboard will be cleared after 1 minute?
Is the plan to purge all clipboard entries after 1 minute, or can we limit that purge just to Private Keys or SRP to be aligned with the mobile iOS client?

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinghim
Copy link
Contributor

Resolves #19948

@danjm
Copy link
Contributor

danjm commented Jul 12, 2023

On the IOS mobile client there is an expectation set for the user by including the following text "stored for 1 minute" when an SRP or private key is copied. Unfortunately, there was not an effective way to enforce the clipboard purge on Android.

Should we inform the user within the UI that the clipboard will be cleared after 1 minute?
Is the plan to purge all clipboard entries after 1 minute, or can we limit that purge just to Private Keys or SRP to be aligned with the mobile iOS client?

Extension product and design are going to look into these as future improvements, but we are going to just change the delay in this PR as an MVP, so we can get it into the v10.34.0 release.

@darkwing darkwing merged commit e329818 into develop Jul 12, 2023
@darkwing darkwing deleted the 19548-copy-address branch July 12, 2023 16:05
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2023
@metamaskbot metamaskbot added the release-10.35.0 Issue or pull request that will be included in release 10.35.0 label Jul 12, 2023
@PeterYinusa PeterYinusa added release-10.34.0 Issue or pull request that will be included in release 10.34.0 and removed release-10.35.0 Issue or pull request that will be included in release 10.35.0 labels Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-10.34.0 Issue or pull request that will be included in release 10.34.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Copying addresses to clipboard is not working
10 participants