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

UX Snaps: Updated dapp connection state for connected snaps #20811

Merged
merged 30 commits into from
Sep 29, 2023

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Sep 11, 2023

This PR is to ensure that when the dapp is connected with a snap, the site connection icon should indicate a connection. Similarly, the currently connected site modal doesn't indicate that the site is connected.

Screenshots/Screencaps

After

Both Account and Snap are connected

Screen.Recording.2023-09-22.at.4.12.32.PM.mov

Only Snap Connected

Screen.Recording.2023-09-22.at.4.12.09.PM.mov

Manual Testing Steps

  • Go to https://snaps.metamask.io/
  • Connect a snap
  • Open MetaMask in popup view, check if the connectivity circle is hollow green
  • Click on the circle, the snap should be in the popover menu.
  • For only snaps connected state, permissions should not be there in modal
  • Go to https://app.walletchat.fun/
  • Sign in using MetaMask
  • Open MetaMask in popup view, check if the connectivity circle should be filled green
  • Click on the circle, both the account and snap should be in the popover menu

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.

@NidhiKJha NidhiKJha added DO-NOT-MERGE Pull requests that should not be merged team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead team-snaps DEPRECATED: Use "team-snaps-platform" or "team-snaps-ecosystem" instead labels Sep 11, 2023
@NidhiKJha NidhiKJha marked this pull request as ready for review September 14, 2023 10:56
@NidhiKJha NidhiKJha requested a review from a team as a code owner September 14, 2023 10:56
@NidhiKJha NidhiKJha removed the DO-NOT-MERGE Pull requests that should not be merged label Sep 14, 2023
@NidhiKJha NidhiKJha changed the title (WIP 👷‍♀️ ) UX Snaps: Updated dapp connection state for connected snaps and no connected Account UX Snaps: Updated dapp connection state for connected snaps and no connected Account Sep 14, 2023
@NidhiKJha NidhiKJha changed the title UX Snaps: Updated dapp connection state for connected snaps and no connected Account UX Snaps: Updated dapp connection state for connected snaps Sep 14, 2023
@eriknson eriknson added the DO-NOT-MERGE Pull requests that should not be merged label Sep 14, 2023
@eriknson
Copy link
Member

Right now it looks like the website is connected with the website. Let's sync & and align on how to design this modal

@NidhiKJha NidhiKJha marked this pull request as draft September 14, 2023 15:56
@NidhiKJha NidhiKJha removed the DO-NOT-MERGE Pull requests that should not be merged label Sep 21, 2023
vthomas13
vthomas13 previously approved these changes Sep 22, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [1f7872d]
Page Load Metrics (1811 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1213591704923
domContentLoaded15362004180915675
load15362057181115977
domInteractive15362004180915675
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 6.31 KiB (0.08%)
  • common: 251 Bytes (0.01%)

vthomas13
vthomas13 previously approved these changes Sep 25, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [2ba5304]
Page Load Metrics (1716 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1221971522411
domContentLoaded139321161716235113
load139321161716235113
domInteractive139221161716235113
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 6.31 KiB (0.08%)
  • common: 251 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [08f8edc]
Page Load Metrics (712 ± 364 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint84156103178
domContentLoaded68144942010
load791909712758364
domInteractive68144942010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 6.28 KiB (0.08%)
  • common: 251 Bytes (0.01%)

Copy link
Contributor

@vthomas13 vthomas13 left a comment

Choose a reason for hiding this comment

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

Looks great!

@NidhiKJha NidhiKJha merged commit 8fcd83b into develop Sep 29, 2023
10 checks passed
@NidhiKJha NidhiKJha deleted the snaps-permission branch September 29, 2023 17:50
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 29, 2023
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-extension-ux DEPRECATED: please use "team-wallet-ux" label instead team-snaps DEPRECATED: Use "team-snaps-platform" or "team-snaps-ecosystem" instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update dapp connection state for accounts connected via Snaps (without connected accounts)
6 participants