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: fetch nfts on components #24547

Merged
merged 42 commits into from
Jun 19, 2024
Merged

feat: fetch nfts on components #24547

merged 42 commits into from
Jun 19, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented May 15, 2024 •
edited
Loading

Description

This PR aims to:
1- Enable nft autodetection by default
2- Show a modal only once if the user disables the nft autodetection
3- Make NFT detection tied to the components that use the NFTs instead of the 3mins polling strategy.

This PR goes with this core PR: MetaMask/core#4281

Open in GitHub Codespaces

Related issues

Related to: MetaMask/core#4281

Manual testing steps

Test the nft auto detection modal:

  1. Do a fresh import of the wallet
  2. Switch to mainnet.
  3. Go to settings => Security and privacy and make sure the toggle enable NFT auto detection is ON
  4. Turn the nft auto detection toggle OFF
  5. Go back to home page and you should see a new modal
  6. Clicking on not right now button should close the modal and closing on allow button should enable the nft auto detection modal.
    This modal should be seen only once.

Test the removal of the polling in the backgound:

We have the toggle enabled by default now but this should not trigger calls to NFT-API every 3 mins anymore.
Instead the calls should be triggered only when you click on the NFT tab.

  1. Open the background console and click on Networks tab.
  2. Filter by /tokens (so you are able to see only the calls that will fetch user nfts)
  3. Notice that as long as you did not click on the NFT tab, you should not be able to see any calls made in the backround.
    (where the old logic should keep detecting your NFTs every 3 mins as long as you have MM open)
  4. Click on the NFT tab and you should be able to see the requests in the background to fetch your nfts.
  5. You can also click on Send, and click on asset picker and click on NFT tab, you should be able to see your NFTs there too.
  6. Calls should be made only when you click on the NFT tab.

Test new notice banner behavior:

Users should see the NFT Notice banner as long as they are on mainnet + they have NFT detection OFF.
Regardless of whether they have NFTs in the state or not.
Clicking on the"Enable NFT autodetection" should remove the notice banner and enable the NFT detection without redirecting the user to settings.

Screenshots/Recordings

Before

Screen.Recording.2024-06-06.at.21.57.21.mov

After

after.mov

Notice banner with toast:

Screen.Recording.2024-06-14.at.13.12.13.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

@sahar-fehri sahar-fehri requested a review from a team as a code owner May 15, 2024 18:41
@sahar-fehri sahar-fehri marked this pull request as draft May 15, 2024 18:41
@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.

@sahar-fehri sahar-fehri force-pushed the feat/refactor-nft-experience branch 2 times, most recently from 88a439f to 5ca4a88 Compare May 30, 2024 14:20
@sahar-fehri sahar-fehri force-pushed the feat/refactor-nft-experience branch from 6afa84e to da18690 Compare June 3, 2024 07:42
@sahar-fehri sahar-fehri mentioned this pull request Jun 3, 2024
Merged
3 tasks
@sahar-fehri sahar-fehri added needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. team-assets labels Jun 6, 2024
@sahar-fehri sahar-fehri marked this pull request as ready for review June 6, 2024 20:13
@metamaskbot
Copy link
Collaborator

Builds ready [0383c5a]
Page Load Metrics (159 ± 228 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6313084189
domContentLoaded9321373
load412228159475228
domInteractive9321373
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 407 Bytes (0.01%)
  • ui: 8.92 KiB (0.13%)
  • common: 3 KiB (0.05%)

@bergeron
Copy link
Contributor

  • Verified autodetection is on by default
  • The modal appeared in all cases i expected (new + existing user)
    • I think clicking allow should also enable the Display NFT media setting - see comment
  • Verified banner appeared with detection off, even when i have NFTs.
  • Verified NFT requests only trigger on UI components, and frequent requests are cached

The one thing I've been reproducing is the lack of a loading spinner. I onboard a wallet with NFTs, and I keep the network tab on "slow 3g" to exaggerate the effect. But it goes straight to "No NFTs yet", during both the request to the NFT API and subsequent RPC calls. Do we expect a loading spinner here?

Screen.Recording.2024-06-12.at.9.28.49.PM.mov

@sahar-fehri
Copy link
Contributor Author

  • Verified autodetection is on by default

  • The modal appeared in all cases i expected (new + existing user)

    • I think clicking allow should also enable the Display NFT media setting - see comment

  • Verified banner appeared with detection off, even when i have NFTs.

  • Verified NFT requests only trigger on UI components, and frequent requests are cached

The one thing I've been reproducing is the lack of a loading spinner. I onboard a wallet with NFTs, and I keep the network tab on "slow 3g" to exaggerate the effect. But it goes straight to "No NFTs yet", during both the request to the NFT API and subsequent RPC calls. Do we expect a loading spinner here?

Screen.Recording.2024-06-12.at.9.28.49.PM.mov

Thanks a lot for the QA :D 🙏 Nice one, would you be able to test again? It should be working now 🙏

@metamaskbot
Copy link
Collaborator

Builds ready [8c85de7]
Page Load Metrics (185 ± 267 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6312591168
domContentLoaded9311463
load432610185556267
domInteractive9311463
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 7.49 KiB (0.11%)
  • common: 2.84 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [002f88d]
Page Load Metrics (49 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6710883115
domContentLoaded9231131
load39714984
domInteractive9231131
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 13 Bytes (0.00%)
  • ui: 8.93 KiB (0.13%)
  • common: 3.33 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [e45ebc8]
Page Load Metrics (48 ± 3 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint681047984
domContentLoaded9211232
load41634873
domInteractive9211232
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 8.93 KiB (0.13%)
  • common: 3.33 KiB (0.05%)

bergeron
bergeron previously approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@bergeron bergeron left a comment

Choose a reason for hiding this comment

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

Looks good! I see loading spinners now, and Display NFT media is enabled when I click allow

@metamaskbot
Copy link
Collaborator

Builds ready [7760571]
Page Load Metrics (139 ± 176 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6015188199
domContentLoaded98915178
load401737139367176
domInteractive98915178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 8.93 KiB (0.13%)
  • common: 3.36 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [666362f]
Page Load Metrics (187 ± 208 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62997694
domContentLoaded8201031
load381589187433208
domInteractive8201031
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 9.1 KiB (0.13%)
  • common: 3.36 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [666362f]
Page Load Metrics (187 ± 208 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62997694
domContentLoaded8201031
load381589187433208
domInteractive8201031
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 9.1 KiB (0.13%)
  • common: 3.36 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [666362f]
Page Load Metrics (187 ± 208 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62997694
domContentLoaded8201031
load381589187433208
domInteractive8201031
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 12 Bytes (0.00%)
  • ui: 9.1 KiB (0.13%)
  • common: 3.36 KiB (0.05%)

@sahar-fehri sahar-fehri merged commit e54295d into develop Jun 19, 2024
74 checks passed
@sahar-fehri sahar-fehri deleted the feat/refactor-nft-experience branch June 19, 2024 20:08
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bergeron bergeron bergeron approved these changes

@salimtb salimtb salimtb approved these changes

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-12.1.0 Issue or pull request that will be included in release 12.1.0 team-assets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sahar-fehri @metamaskbot @bergeron @salimtb