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

feat(snaps): Settings Page #29234

Merged
merged 10 commits into from
Dec 17, 2024
Merged

Conversation

GuillaumeRx
Copy link
Contributor

@GuillaumeRx GuillaumeRx commented Dec 16, 2024

Description

This PR adds a settings section for each preinstalled snaps that exposes a onSettingsPage handler

Open in GitHub Codespaces

Related issues

Fixes: MetaMask/snaps#2874

Manual testing steps

  1. Open MetaMask's settings
  2. You should see the preinstalled snaps settings

Screenshots/Recordings

Before

After

image
image
image

Pre-merge author checklist

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.

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 metamaskbot added the team-snaps-platform Snaps Platform team label Dec 16, 2024
@GuillaumeRx GuillaumeRx force-pushed the gr/snap-settings-page branch from e59b239 to 47c7677 Compare December 17, 2024 14:33
@GuillaumeRx GuillaumeRx changed the base branch from main to fb/snaps-bump-v81 December 17, 2024 14:33
interfaceId={interfaceId}
isLoading={loading}
useDelineator={false}
contentBackgroundColor={BackgroundColor.backgroundDefault}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The settings page having a backgroundDefault background color interferes with some of our components.

Copy link
Member

Choose a reason for hiding this comment

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

Did we fix the clash by forcing a background or is there additional problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clash still exists, the primary problem is with Section that has the same background color

@@ -177,7 +177,7 @@ describe('TypedSignInfo', () => {
type: TransactionType.signTypedData,
chainId: '0x5',
});
(snapUtils.isSnapId as jest.Mock).mockReturnValue(false);
(snapUtils.isSnapId as unknown as jest.Mock).mockReturnValue(false);
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a type error now that the function is written in typescript

@GuillaumeRx GuillaumeRx marked this pull request as ready for review December 17, 2024 15:43
@GuillaumeRx GuillaumeRx requested review from a team as code owners December 17, 2024 15:43
@metamaskbot
Copy link
Collaborator

Builds ready [44fb407]
Page Load Metrics (1782 ± 116 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26627671638512246
domContentLoaded156227531749247118
load158827671782242116
domInteractive26122452713
backgroundConnect1073332010
firstReactRender1679372512
getState55217178
initialActions00000
loadScripts115022891323236113
setupStore665212010
uiStartup181930192019248119
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 217 Bytes (0.00%)
  • ui: 9.21 KiB (0.12%)
  • common: 4.64 KiB (0.06%)

@GuillaumeRx GuillaumeRx merged commit c80f17a into fb/snaps-bump-v81 Dec 17, 2024
14 of 17 checks passed
@GuillaumeRx GuillaumeRx deleted the gr/snap-settings-page branch December 17, 2024 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider letting Snaps expose settings that show up alongside the MetaMask settings
3 participants