-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Confirmation page alerts #20125
Confirmation page alerts #20125
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. |
6e30484
to
1b9539a
Compare
4cd64a6
to
cc03448
Compare
Builds ready [cc44ac4]
Page Load Metrics (1462 ± 31 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov Report
@@ Coverage Diff @@
## blockaid_metrics #20125 +/- ##
====================================================
+ Coverage 68.69% 68.72% +0.02%
====================================================
Files 986 986
Lines 37898 37844 -54
Branches 10148 10122 -26
====================================================
- Hits 26034 26005 -29
+ Misses 11864 11839 -25
|
@metamaskbot update-policies |
No policy changes |
Builds ready [54f27c5]
Page Load Metrics (1606 ± 55 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
628e2e8
to
6603bf7
Compare
6603bf7
to
3c2e7fd
Compare
Builds ready [997c035]
Page Load Metrics (1709 ± 80 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [bd8e6f9]
Page Load Metrics (1666 ± 77 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
No policy changes |
Builds ready [d8192ee]
Page Load Metrics (1734 ± 83 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, the description is
Adding security alert banners on confirmation pages.
can we provide more details please? there also seems to be additional logic added to this PR. It may have been helpful to separate logic into more PRs
example details i think we can add
- update
app/scripts/lib/ppom/ppom.js
- support
result_type: Failed
key/value - rename
ppomResponse
→securityAlertResponse
- update margin
if ( | ||
resultType === BlockaidResultType.Benign || | ||
resultType === BlockaidResultType.Failed | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to our discussion @jpuri we are supposed to return the new banner alert when result type is Failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segun : UI changed for failed
status will be implemented in a following PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be helpful to add in the description. working on it here:
https://github.com/MetaMask/MetaMask-planning/issues/1056
…sk-extension into confirmation_page_alerts
Yep @digiwand : the PR is old and description became stale. |
… confirmation_page_alerts
Builds ready [d88685e]
Page Load Metrics (1610 ± 82 ms)
Bundle size diffs
|
Adding security alert banners on confirmation pages.