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

[Metrics] Add wallet metrics #560

Merged
merged 30 commits into from
Nov 15, 2023
Merged

[Metrics] Add wallet metrics #560

merged 30 commits into from
Nov 15, 2023

Conversation

BrettCleary
Copy link
Collaborator

@BrettCleary BrettCleary commented Sep 28, 2023

Improve wallet metrics in onboarding modal
Update MetaMask Mobile to 0.12.0

  • I believe MMM metrics are automatically enabled on this version but am double checking with the team. This should not block this PR as it will at most be a one line change in the proxy server submodule

Fix MetaMask one time passcode (OTP) bug where it wouldn't show in some cases when reconnecting on subsequent app launches

Flowchart with events:
https://www.figma.com/file/JG7Rbq91JE8JbtvGVFNLPH/Metrics?type=whiteboard&node-id=0%3A1&t=RpQbfJfp1CIvv44j-1

Manual Test Cases are in qase under "Metrics -> Wallet Onboarding"

Submodule PR's

AC

  • Fire the MetaMask Extension event from the MetaMask SDK to help the MetaMask team with tracking
  • tracking events for all onboarding states

@BrettCleary BrettCleary self-assigned this Sep 28, 2023
@BrettCleary BrettCleary added PR: Ready-For-Review PR is ready to be reviewed by peers PR: Ready-For-Test PR is ready to be tested by a QA and removed PR: WIP labels Sep 30, 2023
@BrettCleary BrettCleary marked this pull request as ready for review September 30, 2023 06:59
@eliobricenov eliobricenov self-requested a review October 2, 2023 19:06
Copy link
Contributor

@eliobricenov eliobricenov left a comment

Choose a reason for hiding this comment

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

left some questions

@BrettCleary BrettCleary removed PR: Ready-For-Review PR is ready to be reviewed by peers PR: Ready-For-Test PR is ready to be tested by a QA labels Oct 26, 2023
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this conflicts with our window.ethereum type definition in the frontend even though this is imported in the main process

There is also no reason for this to be exported by the node.js mm sdk. Created an issue here for it MetaMask/metamask-sdk#491

@BrettCleary
Copy link
Collaborator Author

Ready for review again @eliobricenov @flavioislima 👍

<div
className={styles.defaultProfileImage}
style={{
backgroundColor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you maybe turn this into a variable to make to more readable and easy to change?
Also, the color code 202124 on a constant so would be easier to change in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactor. Code is cleaner now using smaller components 👍🏽

Copy link
Contributor

@flavioislima flavioislima 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 overall.
left just some small comments 👍🏽

Copy link
Contributor

@eliobricenov eliobricenov left a comment

Choose a reason for hiding this comment

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

lgtm!

<div className={styles.profileOptionsContainer}>
{getPackageManagersForBrowser().map((pkgManager) => {
if (importOptions[browserSelected]![pkgManager].length === 0)
return null
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, lets at least put another new line under the return null or we wrap it with curly brackets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure I'll add brackets but we should not enforce linting styles through code review. It wastes time. If it passes the linter, it should be okay

Here is the issue for adding eslint curly to enforce this at the linting level #581

Copy link
Contributor

@red-game-dev red-game-dev left a comment

Choose a reason for hiding this comment

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

The above consider them as optional, so please do not feel blocked. LGTM 🙌

@BrettCleary BrettCleary merged commit 67fa89e into main Nov 15, 2023
13 of 14 checks passed
@BrettCleary BrettCleary deleted the metrics/wallet_events branch November 15, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants