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

feat: add announcement for users that blockaid will be enabled by default #22338

Merged
merged 18 commits into from
Jan 11, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Dec 19, 2023

Description

Add announcement that blockaid is now enabled by default.

Related issues

Fixes: MetaMask/MetaMask-planning#1835

Manual testing steps

  1. Reload the extension
  2. Find announcement that blockaid not now available by default

Screenshots/Recordings

Screenshot 2023-12-19 at 6 26 56 PM

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.

@jpuri jpuri added team-confirmations-secure-ux-PR PRs from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 19, 2023
@jpuri jpuri requested a review from a team as a code owner December 19, 2023 12:57
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.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

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

Comparison is base (f76c353) 67.92% compared to head (94e45f1) 67.91%.
Report is 1 commits behind head on develop.

Files Patch % Lines
.../components/app/whats-new-popup/whats-new-popup.js 0.00% 2 Missing ⚠️
shared/notifications/index.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #22338      +/-   ##
===========================================
- Coverage    67.92%   67.91%   -0.00%     
===========================================
  Files         1070     1070              
  Lines        41360    41364       +4     
  Branches     11113    11115       +2     
===========================================
+ Hits         28091    28092       +1     
- Misses       13269    13272       +3     

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

@metamaskbot
Copy link
Collaborator

Builds ready [94e45f1]
Page Load Metrics (1294 ± 152 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint883432058742
domContentLoaded9209908038
load82120371294316152
domInteractive9209908038
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 61 Bytes (0.00%)
  • common: 880 Bytes (0.02%)

@jpuri jpuri changed the title Add announcement for users that blockaid will be enabled by default feat: add announcement for users that blockaid will be enabled by default Jan 5, 2024
digiwand
digiwand previously approved these changes Jan 9, 2024
shared/notifications/index.js Outdated Show resolved Hide resolved
@@ -2971,6 +2971,21 @@
"notifications24Title": {
"message": "Advanced gas fees by network"
},
"notifications29ActionText": {
"message": "Got it"
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a few of instances of this message already. can we re-use the 'gotIt' key for this instead of creating another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if there is a PR open for this yet, but we'll want to update the gotIt translation key for all language files to something like 'gotItExclamation' and condense the instances mentioned to "gotIt"

Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

Changes look good, just a nit about locales.

@HowardBraham
Copy link
Contributor

Can we please use the new format that would replace all those 29s with something like NOTIFICATION_BLOCKAID_DEFAULT?

This new format makes it a lot easier to deal with merge conflicts.

@jpuri
Copy link
Contributor Author

jpuri commented Jan 10, 2024

NOTIFICATION_BLOCKAID_DEFAULT

Thanks @HowardBraham : I made code updates.

@jpuri jpuri requested review from segun and digiwand January 10, 2024 15:02
shared/notifications/index.js Outdated Show resolved Hide resolved
@@ -2971,6 +2971,21 @@
"notifications24Title": {
"message": "Advanced gas fees by network"
},
"notifications29ActionText": {
"message": "Got it"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if there is a PR open for this yet, but we'll want to update the gotIt translation key for all language files to something like 'gotItExclamation' and condense the instances mentioned to "gotIt"

@jpuri jpuri requested a review from digiwand January 10, 2024 17:19
…etamask-extension into blockaid_default_announcement
digiwand
digiwand previously approved these changes Jan 10, 2024
segun
segun previously approved these changes Jan 10, 2024
@jpuri jpuri dismissed stale reviews from digiwand and segun via 9cc54ae January 10, 2024 18:11
@jpuri jpuri requested review from segun and digiwand January 10, 2024 18:22
@HowardBraham
Copy link
Contributor

@jpuri Thanks for the replacement of the 29s, and that merge conflict already happened, pushing this to 30!

I noticed one more thing though. I think all these ///: BEGIN:ONLY_INCLUDE_IF(blockaid) only increase the complexity of the build. You could accomplish the same thing if you removed all of these code fences, then in selectors.js just had
[NOTIFICATION_BLOCKAID_DEFAULT]: whatever_checks_for_blockaid_at_runtime,

@jpuri
Copy link
Contributor Author

jpuri commented Jan 11, 2024

@jpuri Thanks for the replacement of the 29s, and that merge conflict already happened, pushing this to 30!

I noticed one more thing though. I think all these ///: BEGIN:ONLY_INCLUDE_IF(blockaid) only increase the complexity of the build. You could accomplish the same thing if you removed all of these code fences, then in selectors.js just had [NOTIFICATION_BLOCKAID_DEFAULT]: whatever_checks_for_blockaid_at_runtime,

@HowardBraham : all of blockaid code is currently code fenced and I would prefer to keep it this way, also we do not have a corresponding runtime check for code fencing.

@jpuri jpuri merged commit 19a9a5e into develop Jan 11, 2024
67 of 68 checks passed
@jpuri jpuri deleted the blockaid_default_announcement branch January 11, 2024 14:33
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2024
@metamaskbot metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.9.0 Issue or pull request that will be included in release 11.9.0 team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants