fix: alert system and refine SIWE and contract interaction alerts #27205
Conversation
|
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. |
vinistevam
changed the title
Builds ready [8fa2cd6]
Page Load Metrics (1543 ± 72 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #27205 +/- ##
===========================================
+ Coverage 70.01% 70.01% +0.01%
===========================================
Files 1445 1445
Lines 50194 50199 +5
Branches 14041 14043 +2
===========================================
+ Hits 35139 35146 +7
+ Misses 15055 15053 -2 ☔ View full report in Codecov by Sentry. |
app/_locales/en/messages.json
Outdated
| @@ -1019,10 +1019,10 @@ | |||
| "message": "I have acknowledged the alert and still want to proceed" | |||
| }, | |||
| "confirmAlertModalDetails": { | |||
There was a problem hiding this comment.
To change the copy correctly and trigger new translations, you can add the new copy under a new translation key, and delete the old key in all languages.
There was a problem hiding this comment.
Oh, thanks for pointing that out. I've applied the recommended changes. 474affa
|
Builds ready [474affa]
Page Load Metrics (1732 ± 107 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [14d6c72]
Page Load Metrics (1768 ± 85 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [7b18c80]
Page Load Metrics (1866 ± 71 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const [multipleAlertModalVisible, setMultipleAlertModalVisible] = | ||
| useState<boolean>(unconfirmedDangerAlerts.length > 1); | ||
| useState<boolean>(unconfirmedDangerFieldAlerts.length > 0); |
There was a problem hiding this comment.
Do we also want to show the multiple alert modal if there is more than one general alert, since that would mean neither was rendered in the confirmation already?
There was a problem hiding this comment.
We won't show general alerts in the multiple alert modal. @SayaGT asked me to remove the logic behind the general alert when there is more than one.
I'm still working on that and will probably do it in a separate PR cause this one already got too big with all those fixes.
| @@ -117,7 +117,7 @@ export const ConfirmInfoRow: React.FC<ConfirmInfoRowProps> = ({ | |||
| {label} | |||
| </Text> | |||
| {labelChildren} | |||
| {tooltip && tooltip.length > 0 && ( | |||
| {!labelChildren && tooltip && tooltip.length > 0 && ( | |||
There was a problem hiding this comment.
Minor, could we just do !labelChildren && tooltip?.length ?
| isAlertConfirmed, | ||
| } = useAlerts(alertOwnerId); | ||
|
|
||
| const unconfirmedDangerAlertsFields = fieldAlerts.filter( |
There was a problem hiding this comment.
Same question here, do we want to also Review alerts if there is more than one general danger alert since that would mean we show the aggregate message in the confirmation instead?
There was a problem hiding this comment.
Same as above. We may need to revisit this in DSU because it means we'll be back to stacking banner alerts.
|
Description
This PR aims to apply fixes and improvements to the Alert System in general and specific alerts such as SIWE and contract interaction.
Fixes:
1- Display a more generic and toned-down message on the friction modal.
2- Show alerts before the Confirm Alert Modal (final friction modal) when there is a danger alert.
3- Fixed CTA button when the alert has not been acknowledged.
Fixes from UX QA feedback
Figma
security-searchicon when button isReview AlertsorReview Alert- ✔️Xbutton - ✔️Related issues
Fixes: #27132 #27147 #27118 #27238
Manual testing steps
Case 1
Case 2
Case 3
Case 4
Screenshots/Recordings
alert-fix.webm
mint-without-funds.webm
Contract.Interaction.and.Alerts.webm
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist