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.

Sign up for GitHub

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

Jump to bottom

feat: Integrate JSX into snap notifications #27407

Open
wants to merge 64 commits into
base: develop
Choose a base branch
from
Open

feat: Integrate JSX into snap notifications #27407

wants to merge 64 commits into from

Conversation

hmalik88
Copy link
Contributor

@hmalik88 hmalik88 commented Sep 25, 2024 •
edited
Loading

Description

Adds an expanded view for snaps notifications, using JSX content returned from the snap to populate the expanded view.

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions:

  1. What is the reason for the change? Allow for richer snaps notifications
  2. What is the improvement/solution? Add expanded view for snaps, allowing a snap to return jsx content in the expanded view.

Screenshots/Recordings

After

Screen.Recording.2024-10-14.at.7.24.59.AM.mov

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.

@github-actions 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.

@github-actions github-actions bot added the team-snaps-platform Snaps Platform team label Sep 25, 2024
@hmalik88 hmalik88 changed the base branch from mrtenz/bump-snaps-packages to develop September 25, 2024 19:42
@socket-security Socket Security
Copy link

socket-security bot commented Sep 25, 2024 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Mrtenz and others added 11 commits September 27, 2024 17:16
@Mrtenz
Bump dependencies again
bdd1935
@hmalik88
update notification controller, update raw snap notification type
0e30d3a
@hmalik88
Merge branch 'develop' into hm/integrate-jsx-notifications
4597ba1
@hmalik88
Merge branch 'mrtenz/bump-snaps-packages' into hm/integrate-jsx-notif…
5689fe5 
…ications
@hmalik88
add snap notification modal
3d5a9c4
@hmalik88
Merge branch 'develop' into mrtenz/bump-snaps-packages
1dfa869
@GuillaumeRx
feat(snaps): Connect getCurrencyRate hook to `multichainRateControl…
306d898 
…ler` (#27489)
@hmalik88
feat: Integrate MetaMask links into Snaps Link component (#27377)
f375215 
## **Description**


Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change? Previously, developers couldn't
navigate to parts of the extension with the existing functionality
provided to them through snaps UI
2. What is the improvement/solution? The Snaps `Link` component now
checks for if the url is a MetaMask url and navigates to the proper path
accordingly.


## **Screenshots/Recordings**

### **After**


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
@hmalik88
Merge branch 'mrtenz/bump-snaps-packages' into hm/integrate-jsx-notif…
a6b6016 
…ications
@hmalik88
Merge branch 'mrtenz/bump-snaps-packages' into hm/integrate-jsx-notif…
8377776 
…ications
@hmalik88
integrate snaps expanded view into notification system
aad36db
@hmalik88 hmalik88 changed the base branch from develop to mrtenz/bump-snaps-packages October 8, 2024 03:54
hmalik88
hmalik88 commented Oct 8, 2024
View reviewed changes
ui/pages/notifications/notification-components/index.ts Outdated
Comment on lines 14 to 25
export enum SNAP_TRIGGER {
SNAP = 'snap',
}

export const TRIGGER_TYPES = {
...NotificationServicesController.Constants.TRIGGER_TYPES,
...SNAP_TRIGGER,
};

export type TRIGGER_TYPES =
| NotificationServicesController.Constants.TRIGGER_TYPES
| SNAP_TRIGGER;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusting types here to accommodate for snaps, these will be adjusted upstream in a later PR.

Mrtenz and others added 6 commits October 11, 2024 12:31
@Mrtenz
Merge branch 'develop' into mrtenz/bump-snaps-packages
7c18e47
@GuillaumeRx
feat(snaps): Add size prop to Heading (#27721)
fbe1ca7 
## **Description

This PR adds support for the `size` prop to the `Heading` component.
Right now `md` is bind to `headingMd`, `lg` is bind to `headingLg` to
match
https://www.figma.com/design/2cqqTWJKoHcjYVGfNXxLbQ/Beyond-Ethereum?node-id=3458-5041&node-type=text&m=dev
as requested by @eriknson. The default behavior if `size` is not defined
si `headingSm`

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27721?quickstart=1)

## **Related issues**

## **Manual testing steps**

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
@hmalik88
lint fix
7de9c14
@hmalik88
import fix
3b6141e
@hmalik88
another fix
74aa414
@hmalik88
Merge branch 'mrtenz/bump-snaps-packages' into hm/integrate-jsx-notif…
bc47345 
…ications
@hmalik88 hmalik88 marked this pull request as ready for review October 11, 2024 14:50
@hmalik88 hmalik88 requested review from a team as code owners October 11, 2024 14:50
@metamaskbot
Copy link
Collaborator

Builds ready [bc47345]
Page Load Metrics (2202 ± 222 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint38141092121643309
domContentLoaded168830752051323155
load174836112202463222
domInteractive18146563316
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -49.14 KiB (-1.22%)
  • ui: 7.27 KiB (0.09%)
  • common: 11.14 KiB (0.15%)

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
11.2% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Oct 14, 2024
@Mrtenz Mrtenz requested a review from a team as a code owner October 14, 2024 15:16
@Mrtenz Mrtenz force-pushed the mrtenz/bump-snaps-packages branch 3 times, most recently from 893d11b to 7598b3a Compare October 15, 2024 12:32
Base automatically changed from mrtenz/bump-snaps-packages to develop October 16, 2024 19:35
@metamaskbot
Copy link
Collaborator

Builds ready [cc5a9e2]
Page Load Metrics (2253 ± 185 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29833032131554266
domContentLoaded173326132115276133
load176134562253385185
domInteractive259554199
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -49.15 KiB (-1.22%)
  • ui: 7.74 KiB (0.10%)
  • common: 11.26 KiB (0.15%)

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

@FrederikBolding FrederikBolding FrederikBolding left review comments

@Mrtenz Mrtenz Mrtenz requested changes

@GuillaumeRx GuillaumeRx GuillaumeRx left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-snaps-platform Snaps Platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hmalik88 @Mrtenz @metamaskbot @FrederikBolding @GuillaumeRx