Skip to content

feat: MCv1 Permissions: Site Connections Tooltip#23189

Merged
vthomas13 merged 11 commits intodevelopfrom
1980-tooltip
Mar 7, 2024
Merged

feat: MCv1 Permissions: Site Connections Tooltip#23189
vthomas13 merged 11 commits intodevelopfrom
1980-tooltip

Conversation

@vthomas13
Copy link
Copy Markdown
Contributor

@vthomas13 vthomas13 commented Feb 26, 2024 •
edited
Loading

Description

A follow-up PR to add tooltips to ConnectionListItem.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1980

Manual testing steps

  1. Open Metamask
  2. Connect to a dapp on at least 8 accounts
  3. Go To "All Permissions" screen from the global menu item
  4. Hover over an Account AvatarGroup
  5. Verify that the tooltip shows a maximum of 7 accounts with blockies/jazzicons and has a "+X more" representing the accounts we could not show in the tooltip.

Screenshots/Recordings

Before

No tooltip

After

image

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 26, 2024

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.

@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Feb 26, 2024

Look, just because I'm turning 40 this year isn't any reason to bring the 80's into this repo.

@vthomas13 vthomas13 added team-extension-ux team-core-extension-ux Core Extension UX team needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Feb 27, 2024
@vthomas13 vthomas13 changed the title 1980 tooltip feat: MCv1 Permissions: Site Connections Tooltip Feb 27, 2024
@vthomas13 vthomas13 marked this pull request as ready for review February 27, 2024 22:28
@vthomas13 vthomas13 requested a review from a team as a code owner February 27, 2024 22:28
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 27, 2024

Builds ready [f4c4145]
Page Load Metrics (1227 ± 400 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint702271484321
domContentLoaded1178382211
load5719291227832400
domInteractive1178382211
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.97 KiB (0.03%)
  • common: 131 Bytes (0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 68.67%. Comparing base (dec5421) to head (06ec366).

Files Patch % Lines
...connection-list-tooltip/connection-list-tooltip.js 87.50% 2 Missing ⚠️
ui/selectors/selectors.js 66.67% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23189      +/-   ##
===========================================
+ Coverage    68.66%   68.67%   +0.01%     
===========================================
  Files         1105     1106       +1     
  Lines        43338    43353      +15     
  Branches     11586    11590       +4     
===========================================
+ Hits         29758    29771      +13     
- Misses       13580    13582       +2

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

NidhiKJha
NidhiKJha requested changes Feb 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

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

Looking good. But I think we need to refactor the code here as this is gonna be a new component https://www.notion.so/metamask-consensys/Amon-Hen-components-readiness-check-in-9dd393e209004d079e26d4fffea80f88?pvs=4#3f5e7ae6ba2f4e8586ddb5f58dd0211e

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 28, 2024

Builds ready [6b6e251]
Page Load Metrics (793 ± 418 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint571671102613
domContentLoaded105123115
load452074793870418
domInteractive105123115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.97 KiB (0.03%)
  • common: 131 Bytes (0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 29, 2024

Builds ready [4969ae6]
Page Load Metrics (803 ± 476 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint662911346330
domContentLoaded996332814
load532496803991476
domInteractive996332814
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.97 KiB (0.03%)
  • common: 131 Bytes (0.00%)

@vthomas13 vthomas13 requested a review from NidhiKJha February 29, 2024 20:43
@vthomas13 vthomas13 force-pushed the 1980-tooltip branch 2 times, most recently from 6031ae4 to d7aa769 Compare March 1, 2024 15:37
@metamaskbot metamaskbot added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 1, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 1, 2024

Builds ready [d7aa769]
Page Load Metrics (1251 ± 415 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint782221394321
domContentLoaded137433178
load6521331251863415
domInteractive137433178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.6 KiB (0.05%)
  • common: 131 Bytes (0.00%)

DDDDDanica
DDDDDanica reviewed Mar 1, 2024
View reviewed changes
...ponents/multichain/pages/permissions-page/connection-list-tooltip/connection-list-tooltip.js
@@ -0,0 +1,128 @@
import React from 'react';
Copy link
Copy Markdown
Contributor

@DDDDDanica DDDDDanica Mar 1, 2024

Choose a reason for hiding this comment

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

Can we have it typescript one?

Copy link
Copy Markdown
Contributor Author

@vthomas13 vthomas13 Mar 1, 2024

Choose a reason for hiding this comment

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

This may have to be addressed in a future PR. I have a WIP branch based off this branch, so it's really an awkward time to do TS conversion. Definitely afterwards though.

Copy link
Copy Markdown
Contributor

@DDDDDanica DDDDDanica Mar 5, 2024

Choose a reason for hiding this comment

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

Can you add a ticket for this conversion ticket and tackle it after another branch?

darkwing
darkwing previously approved these changes Mar 5, 2024
View reviewed changes
DDDDDanica
DDDDDanica previously approved these changes Mar 5, 2024
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 5, 2024

Builds ready [80ab9e3]
Page Load Metrics (954 ± 427 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint632171053617
domContentLoaded106324178
load582184954889427
domInteractive106324178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.6 KiB (0.05%)
  • common: 131 Bytes (0.00%)

NidhiKJha
NidhiKJha previously approved these changes Mar 6, 2024
View reviewed changes
darkwing
darkwing previously approved these changes Mar 7, 2024
View reviewed changes
@vthomas13 vthomas13 dismissed stale reviews from NidhiKJha and darkwing via 51e0680 March 7, 2024 17:40
@vthomas13 vthomas13 merged commit 65769f1 into develop Mar 7, 2024
@vthomas13 vthomas13 deleted the 1980-tooltip branch March 7, 2024 19:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 7, 2024
@metamaskbot metamaskbot added the release-11.14.0 Issue or pull request that will be included in release 11.14.0 label Mar 7, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 7, 2024

Builds ready [06ec366]
Page Load Metrics (1126 ± 405 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint771731223014
domContentLoaded1079312010
load6419771126843405
domInteractive1079312010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.63 KiB (0.05%)
  • common: 131 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DDDDDanica DDDDDanica Awaiting requested review from DDDDDanica

@NidhiKJha NidhiKJha Awaiting requested review from NidhiKJha

2 more reviewers

@darkwing darkwing darkwing approved these changes

@jonybur jonybur jonybur approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-11.14.0 Issue or pull request that will be included in release 11.14.0 team-core-extension-ux Core Extension UX team

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vthomas13 @brad-decker @metamaskbot @darkwing @jonybur @DDDDDanica @NidhiKJha