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: add tooltip description to manageAccounts method #20964

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Sep 19, 2023

Description

This PR adds the missing tooltip description to the manageAccounts permission.

Snap permissions source.

Tooltip updated to: Allow this Snap to add or remove Ethereum accounts, then transact and sign with these accounts.

Review comment from @coreyjanssen

Manual testing steps

1. Go to https://metamask.github.io/snap-simple-keyring/0.2.2/
2. Proceed to install snap using Flask
3. Hover tooltip icon. It should show the permission description
4. Install snap
5. Go to snap settings
6. See details of the manageAccounts permission. It should show the permission description

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change.

Screenshot 2023-09-21 at 13 59 27 Screenshot 2023-09-21 at 14 00 02

Related issues

Fixes https://github.com/MetaMask/accounts-planning/issues/52

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes https://github.com/MetaMask/accounts-planning/issues/52
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@gantunesr gantunesr added team-accounts needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 19, 2023
@gantunesr gantunesr marked this pull request as ready for review September 19, 2023 22:09
@gantunesr gantunesr requested a review from a team as a code owner September 19, 2023 22:09
danroc
danroc previously approved these changes Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.16% ⚠️

Comparison is base (a0349a6) 68.40% compared to head (3c39a78) 68.25%.

❗ Current head 3c39a78 differs from pull request most recent head 80e8de8. Consider uploading reports for the commit 80e8de8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20964      +/-   ##
===========================================
- Coverage    68.40%   68.25%   -0.16%     
===========================================
  Files         1006     1006              
  Lines        40217    40184      -33     
  Branches     10758    10742      -16     
===========================================
- Hits         27510    27424      -86     
- Misses       12707    12760      +53     
Files Changed Coverage Δ
ui/helpers/utils/permission.js 16.13% <ø> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [3c39a78]
Page Load Metrics (1633 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint111167141178
domContentLoaded14352303163317182
load14352303163317182
domInteractive14352303163317182
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 222 Bytes (0.00%)

@HowardBraham
Copy link
Contributor

All of the other tooltips start with "Allow"
To be more consistent, can we phrase this as something like:

Allow the snap to add Ethereum accounts, remove Ethereum accounts, and transact/sign with these accounts.

@gantunesr
Copy link
Member Author

gantunesr commented Sep 20, 2023

I agree that would be better, but we need @eriknson's approval to change it

@coreyjanssen
Copy link
Contributor

Slight revision:
Allow this Snap to add or remove Ethereum accounts, then transact and sign with these accounts.

@HowardBraham HowardBraham merged commit 7dbb8c8 into develop Sep 21, 2023
9 checks passed
@HowardBraham HowardBraham deleted the fix/add-tooltip-manageAccounts branch September 21, 2023 19:16
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 21, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [80e8de8]
Page Load Metrics (1691 ± 75 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1222131512412
domContentLoaded14812038169115675
load14812038169115675
domInteractive14812038169015675
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 224 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.3.0 Issue or pull request that will be included in release 11.3.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants