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

fix: 🏷️ update the text in the popup to enable notifications #26026

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

matteoscurati
Copy link
Contributor

@matteoscurati matteoscurati commented Jul 22, 2024

Description

This PR fixes the reported bug: #25994. The PR changes the text to inform the user about the option to disable notifications from the notifications settings page (see images). The ability to enable/disable notifications directly from the general settings is expected to be implemented in future releases.

Screenshot 2024-07-22 at 22 37 30 Screenshot 2024-07-22 at 22 37 38

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

@matteoscurati matteoscurati added the team-notifications Notifications team label Jul 22, 2024
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.

@matteoscurati matteoscurati marked this pull request as ready for review July 22, 2024 20:42
@matteoscurati matteoscurati requested a review from a team as a code owner July 22, 2024 20:42
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Updated the notification popup text to provide clearer instructions on enabling and disabling notifications.

  • Modified app/_locales/en/messages.json to direct users to the 'notifications settings' instead of 'Settings > Notifications'.
  • Improved user guidance to reduce confusion regarding notification settings.
  • No potential security issues or logical errors introduced by this change.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@@ -6231,7 +6231,7 @@
"message": "To use wallet notifications, we use a profile to sync some settings across your devices. $1"
},
"turnOnMetamaskNotificationsMessageThird": {
"message": "You can turn off notifications at any time in $1"
"message": "You can turn off notifications at any time in the $1"
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit - you can ignore), maybe a description might be handy to inform what the $1 is used for. Realistically we will be re-working this modal in the very-near future to add the notification settings page.

Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Jul 23, 2024

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Updated the notification popup text to provide clearer instructions on enabling and disabling notifications, and improved block explorer URL handling.

  • Locale Updates: Modified multiple locale files (e.g., app/_locales/en/messages.json, app/_locales/de/messages.json) to correct notification settings instructions.
  • Block Explorer Button: Updated ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx to use blockExplorerConfig for dynamic button text.
  • New Constants: Added ui/helpers/constants/metamask-notifications/metamask-notifications.ts to map blockchain networks to block explorers.
  • Utility Enhancements: Updated ui/helpers/utils/notification.util.ts to improve block explorer URL handling and added a type guard function.

18 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@metamaskbot
Copy link
Collaborator

Builds ready [0c6e21e]
Page Load Metrics (254 ± 259 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint77140105168
domContentLoaded96928157
load421895254540259
domInteractive96828157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 2 Bytes (0.00%)

@matteoscurati matteoscurati merged commit d3b9943 into develop Jul 23, 2024
76 of 77 checks passed
@matteoscurati matteoscurati deleted the fix/incorrect-text-in-the-notification-popup branch July 23, 2024 10:27
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
@metamaskbot metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants