Skip to content

Conversation

@darkwing
Copy link
Contributor

@darkwing darkwing commented Jan 24, 2024

Description

Adds event tracking for the new "Buy", "Receive", and "NFT" banners.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1949

Manual testing steps

  1. Create a new wallet (will have 0 balance, 0 NFTs)
  2. Open ui/contexts/metametrics.js and in the trackEvent function, add a console.log to see the events get recorded (or use Logpoints in the debugger)

Screenshots/Recordings

Before

N/A

After

N/A

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

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

@darkwing darkwing force-pushed the 1949-asset-list-event-tracking branch from 4ba64f8 to 0d3845e Compare January 24, 2024 15:50
@darkwing darkwing marked this pull request as ready for review January 26, 2024 18:30
@darkwing darkwing force-pushed the 1949-asset-list-event-tracking branch from d89aadb to b73cc6e Compare January 31, 2024 19:19
NidhiKJha
NidhiKJha previously approved these changes Feb 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [d40792c]
Page Load Metrics (803 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96151117157
domContentLoaded10361763
load7229398035426
domInteractive10361763
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.57 KiB (0.04%)
  • common: 338 Bytes (0.01%)

Comment on lines +138 to +150
chain_id: currentNetwork.chainId,
locale: currentLocale,
network: currentNetwork.nickname,
referrer: ORIGIN_METAMASK,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we can extract them to a property object and use it for different events tracked in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that in a follow up as it's not core and I do kind of like the context of what the nickname and chainId belong to!

@darkwing darkwing force-pushed the 1949-asset-list-event-tracking branch from d40792c to 93a3a68 Compare February 7, 2024 19:35
@codecov
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (642f6d4) 68.47% compared to head (93a3a68) 68.49%.
Report is 7 commits behind head on develop.

Files Patch % Lines
ui/components/app/nfts-tab/nfts-tab.js 70.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #22654      +/-   ##
===========================================
+ Coverage    68.47%   68.49%   +0.02%     
===========================================
  Files         1089     1089              
  Lines        42905    42926      +21     
  Branches     11426    11429       +3     
===========================================
+ Hits         29378    29400      +22     
+ Misses       13527    13526       -1     

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

@metamaskbot
Copy link
Collaborator

Builds ready [93a3a68]
Page Load Metrics (818 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85133102147
domContentLoaded9301863
load73511148188742
domInteractive9301863
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.28 KiB (0.03%)
  • common: 338 Bytes (0.01%)

@darkwing darkwing added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Feb 8, 2024
@darkwing darkwing merged commit 6db80f1 into develop Feb 8, 2024
@darkwing darkwing deleted the 1949-asset-list-event-tracking branch February 8, 2024 22:04
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2024
@metamaskbot metamaskbot added the release-11.12.0 Issue or pull request that will be included in release 11.12.0 label Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-11.12.0 Issue or pull request that will be included in release 11.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants