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

refactor: extract Send-specific functionality out of AssetPicker #26558

Merged
merged 13 commits into from
Aug 29, 2024

Conversation

micaelae
Copy link
Member

@micaelae micaelae commented Aug 20, 2024

Description

The goal of this PR is to make the AssetPicker component experience-agnostic so that it can be reused within other experiences.

Since the AssetPicker was initially built for the Swap+Send experience, most changes here are for moving send-specific logic out of the component, including:

  • move Send event tracking callbacks to send page
  • add new AssetPicker props for setting custom modal header and visible tabs
  • define an experience-agnostic asset prop, which contains the minimal required information about the selected asset
  • use accurate type definitions for assets and props

Open in GitHub Codespaces

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/METABRIDGE-890

Manual testing steps

  1. Swap+Send asset selection experience should not change

Screenshots/Recordings

Before -> After

Screenshot 2024-08-20 at 6 50 34 PM Screenshot 2024-08-20 at 6 52 10 PM

Screenshot 2024-08-20 at 6 50 44 PM Screenshot 2024-08-20 at 6 52 18 PM

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.

@micaelae micaelae requested a review from a team as a code owner August 20, 2024 23:49
@micaelae micaelae marked this pull request as draft August 20, 2024 23:49
@micaelae micaelae force-pushed the mb890-decouple-send-and-asset-picker branch 2 times, most recently from 3819a56 to 4430252 Compare August 21, 2024 01:14
@metamaskbot
Copy link
Collaborator

Builds ready [4430252]
Page Load Metrics (70 ± 17 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint61248964120
domContentLoaded38210663718
load45210703617
domInteractive9114292412
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 2.46 KiB (0.03%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 72.38095% with 29 lines in your changes missing coverage. Please review.

Project coverage is 70.09%. Comparing base (fb61b0f) to head (4dbfad9).
Report is 83 commits behind head on develop.

Files with missing lines Patch % Lines
.../asset-picker-modal/asset-picker-modal-nft-tab.tsx 36.36% 7 Missing ⚠️
...ichain/asset-picker-amount/asset-picker-amount.tsx 70.59% 5 Missing ⚠️
ui/components/multichain/pages/send/send.js 16.67% 5 Missing ⚠️
...r-amount/asset-picker-modal/asset-picker-modal.tsx 87.50% 3 Missing ⚠️
.../asset-picker-amount/asset-picker/asset-picker.tsx 76.92% 3 Missing ⚠️
...ichain/pages/send/components/recipient-content.tsx 75.00% 3 Missing ⚠️
...unt/asset-picker-modal/asset-picker-modal-tabs.tsx 83.33% 2 Missing ⚠️
...t/asset-picker-modal/asset-picker-modal-search.tsx 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26558      +/-   ##
===========================================
+ Coverage    70.08%   70.09%   +0.01%     
===========================================
  Files         1413     1416       +3     
  Lines        49318    49352      +34     
  Branches     13780    13786       +6     
===========================================
+ Hits         34562    34593      +31     
- Misses       14756    14759       +3     

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

@micaelae micaelae force-pushed the mb890-decouple-send-and-asset-picker branch from 4430252 to 5a0f942 Compare August 28, 2024 23:13
@metamaskbot
Copy link
Collaborator

Builds ready [5a0f942]
Page Load Metrics (1755 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22621411609476229
domContentLoaded15802132173012761
load15882140175514570
domInteractive18103402512
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 2.47 KiB (0.03%)
  • common: 0 Bytes (0.00%)

@micaelae micaelae marked this pull request as ready for review August 29, 2024 00:41
@sahar-fehri
Copy link
Contributor

LGTM! Thank you 🙏

Copy link

sonarcloud bot commented Aug 29, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [4dbfad9]
Page Load Metrics (1640 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24719081573323155
domContentLoaded14851900160910048
load14941909164010048
domInteractive146431126
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 2.47 KiB (0.03%)
  • common: 0 Bytes (0.00%)

@micaelae micaelae merged commit f086572 into develop Aug 29, 2024
78 checks passed
@micaelae micaelae deleted the mb890-decouple-send-and-asset-picker branch August 29, 2024 20:30
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 29, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-bridge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants