Replaced ActionableMessage and Deprecated Button #20443
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. |
georgewrmarshall
added
the
team-design-system
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #20443 +/- ##
===========================================
- Coverage 68.82% 68.81% -0.00%
===========================================
Files 1034 1034
Lines 41166 41166
Branches 10994 10994
===========================================
- Hits 28329 28327 -2
- Misses 12837 12839 +2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @pritam1813, great start I've left a couple suggestions.
| <Button | ||
| type="link" | ||
| variant={BUTTON_VARIANT.LINK} | ||
| key="confirm-add-suggested-token-duplicate-warning" | ||
| className="confirm-add-suggested-token__link" |
There was a problem hiding this comment.
Can we remove this class and removed the associated styles
&__link {
@include H7;
display: inline;
color: var(--color-primary-default);
padding-left: 0;
}
from ui/pages/confirm-add-suggested-token/index.scss
We want the link here to be the same size as the body text
| import ActionableMessage from '../../components/ui/actionable-message/actionable-message'; | ||
| import Button from '../../components/ui/button'; | ||
| import { | ||
| BUTTON_VARIANT, |
There was a problem hiding this comment.
I believe BUTTON_VARIANT has now been deprecated in favor of the ButtonVariant enum. Could you please replace?
There was a problem hiding this comment.
Ok, replaced the enum.
There was a problem hiding this comment.
Currently, I am facing this bug #19818, as a result I couldn't update the screenshot. Any suggestion on how to avoid this bug ? I am on Ubuntu.
There was a problem hiding this comment.
Hey @kumavis, would it be possible to support @pritam1813 on his lavamoat issue?
There was a problem hiding this comment.
This is looking great @pritam1813! Made a small suggestion to add some margins so it adds some space between the text and banner alert
| return ( | ||
| hasDuplicateAddress(suggestedTokens, tokens) && ( | ||
| <ActionableMessage | ||
| message={t('knownTokenWarning', [ | ||
| <BannerAlert severity={Severity.Warning}> |
There was a problem hiding this comment.
Let's add a margin top here
| <BannerAlert severity={Severity.Warning}> | |
| <BannerAlert severity={Severity.Warning} marginTop={4}> |
There was a problem hiding this comment.
LGTM! Thanks for your contribution @pritam1813
- pulled branch and tested locally
Explanation
ActionableMessagewithBannerAlertinui/pages/confirm-add-suggested-token/confirm-add-suggested-token.js<Button />component (ui/components/component-library) with new<Button />component (ui/components/component-library/button/button.js) inside theBannerAlertcomponent.Screenshots/Screencaps
Before
Token With Same Symbol But Different Address (Before)
Token With Duplicate Contract Address (Before)
After
Token With Same Symbol But Different Address (After)
Token With Duplicate Contract Address (After)
Manual Testing Steps
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.