-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Adding alerts for transaction inside deprecation #21193
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. |
…etamask-extension into trx_insight_deprecation_alert
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #21193 +/- ##
========================================
Coverage 68.64% 68.64%
========================================
Files 1017 1018 +1
Lines 40788 40791 +3
Branches 10896 10896
========================================
+ Hits 27997 28000 +3
Misses 12791 12791
☔ View full report in Codecov by Sentry. |
Builds ready [3a6321e]
Page Load Metrics (833 ± 392 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
<Box marginLeft={4} marginRight={4} marginTop={4}> | ||
<TransactionInsightsDeprecationAlert /> | ||
</Box> | ||
<Box color={Color.textAlternative} className="confirm-data" padding={4}> |
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.
[nit] we can move `className="confirm-data" to the parent Box element to follow the pattern we have in our codebase. This would also require us to update one of the styles
& > .disclosure {
→ & .disclosure {
(or specifying a new classname)
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.
This is existing div only wrapped in a parent element and an alert placed on top of it. I would avoid refactoring it and functionality is being deprecated.
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.
Yeah, I don't think there is much reason to spend time maintaining this if we are gonna remove it in a couple of releases.
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.
Code LGTM, but is it intentional that it doesn't match the designs? @jpuri
Designs has the deprecation alert at the bottom but here it is implemented at the top 🤔
EDIT: My bad, the screenshots on the issue were out of date.
Description
Add transaction insight deprecation notice
Screenshots/Recordings
If applicable, add screenshots and/or recordings to visualize the before and after of your change.
Related issues
_Fixes MetaMask/MetaMask-planning#1326
Pre-merge author checklist
Pre-merge reviewer checklist