feat: Implement alerts for personal sign#24469
Merged
vinistevam merged 204 commits intodevelopfrom May 28, 2024
Merged
Conversation
…lert-modal-component
…lert-modal-component
…ne-alert-info-row-component
…ne-alert-info-row-component
…al.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
…al.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
…al.test.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
…lert-modal-component
jpuri
reviewed
May 24, 2024
| const hasUnconfirmedAlerts = alerts.some( | ||
| (alert: Alert) => | ||
| !isAlertConfirmed(alert.key) && alert.severity === Severity.Danger, | ||
| ); |
Contributor
There was a problem hiding this comment.
It might be useful to move methods like above to useAlerts so they can be re-used.
Contributor
Author
There was a problem hiding this comment.
100%, I should have done that before. I didn't push it because I saw this improvement in your PR.
jpuri
reviewed
May 24, 2024
| } | ||
| return highestSeverity; | ||
| }, Severity.Info); | ||
| } |
Contributor
There was a problem hiding this comment.
Above may be just a forEach will be good.
jpuri
reviewed
May 24, 2024
| /> | ||
| </Box> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
It will be useful to extract above component into separate file. We try to keep confirmation code lesser number of lines per file and smaller well defined components.
Contributor
Author
There was a problem hiding this comment.
This makes sense, is it ok to extract in a different PR? I'm happy to do it.
ui/pages/confirmations/components/confirm/alert-system/alert-provider/alert-provider.tsx
Show resolved
Hide resolved
jpuri
previously approved these changes
May 24, 2024
Contributor
jpuri
left a comment
There was a problem hiding this comment.
I left some small feedbacks, changes largely look good.
Collaborator
Builds ready [555e858]
Page Load Metrics (1426 ± 599 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
matthewwalsh0
previously approved these changes
May 24, 2024
jpuri
previously approved these changes
May 27, 2024
Collaborator
Builds ready [3e092e3]
Page Load Metrics (788 ± 528 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
6568ea8 to
fc35032
Compare
Collaborator
Builds ready [fc35032]
Page Load Metrics (554 ± 470 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [05041ce]
Page Load Metrics (414 ± 392 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
pedronfigueiredo
approved these changes
May 28, 2024
matthewwalsh0
approved these changes
May 28, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR aims to implement alerts for personal sign.
Context
The confirmation supporting the personal sign operation was one of the first confirmations implemented using the new confirmation architecture.
In order to test and demonstrate the use of the new alert system, we can incorporate suitable alerts into the personal sign confirmation.
Changes
Implementation
personal-sign.tsxto use theAlertRowcomponent.PersonalSignAlertActionsusePersonalSignAlertsto aggregate the alerts from the providers and add those into the new alert system.setConfirmationAlertshook to update and clean alerts based on the current confirmation.useConfirmationAlertshook to integrate the types of confirmations into the new alert system.SecurityAlertBannerin thetitle.tsxto render the general alerts coming from the new alert system.New Banner Alert component
SecurityAlertBannerto support general alerts coming from a provider. This component was based on theSecurityProviderBannerAlert.Folder reorganization
confirmations/alertstoalerts-system(ui/components/app/should not have several subfolders)QA Design feedback
AlertModalonly alerts tied to a field, general alerts should be displayed in the banner on top of the confirmation.infoandwarning.AlertModalbe in the same level of the navigation and close buttons.AlertModal.Clean up, team feedback, QA tests and fixes
AlertSeveritytype to use in theAlerttype, also added aprovideroptional property.handleCloseModalin thefooter.tsx.getBannerAlertSeverity,getProviderAlertSeverityandproviderAlertNormalizerfunctions and added Unit tests.undefinedto use primarily the severity from the alert.handleAcknowledgeClickRelated issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2412
Manual testing steps
PRs Related
#23719
#23625
#23664
#24049
#24214
#24400
Screenshots/Recordings
personal-sign-alert-system.webm
Security banner alert
Multiple alerts
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist