Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace deprecated ActionableMessage component with new BannerAlert component #19528

Open
georgewrmarshall opened this issue Jun 8, 2023 · 11 comments
Open

Replace deprecated ActionableMessage component with new BannerAlert component #19528

georgewrmarshall opened this issue Jun 8, 2023 · 11 comments
Labels
good first issue Good for newcomers team-design-system All issues relating to design system in Extension

Comments

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Jun 8, 2023 •
edited
Loading

Description

Currently, the extension is using an outdated ActionableMessage component, which needs to be replaced with the new BannerAlert component.

This is a massive undertaking by itself and creating a single PR would be too large. Smaller PRs can be submitted against this issue to ensure easier review and gradual improvements.

BannerAlert documentation in storybook

Technical Details

  • Replace instances of ActionableMessage component (ui/components/ui/actionable-message/actionable-message.js) with BannerAlert component (ui/components/component-library/banner-alert/banner-alert.js)
  • Component APIs are slightly different so ensure all props have been migrated appropriately

Acceptance Criteria

  • Instances of ActionableMessage component are completely replaced with the new BannerAlert component
  • The component APIs are updated to reflect the changes in the new ActionableMessage component and there is no functional changes or visual regression
  • Each Pull Request (PR) should include no more than 1 file
  • The code changes should pass Jest tests, linting, and Storybook without any errors.
  • The PR must include before and after screenshots of the UI to ensure there are no visual regressions.

If the acceptance criteria is not met, PRs may be closed.

Difficulty: Intermediate

Good first issue for: External contributors who are familiar with running the extension locally, have knowledge of React, component props, Jest tests, linting, and Storybook, and want to contribute to improving the cohesiveness of UI in the extension
@georgewrmarshall georgewrmarshall added good first issue Good for newcomers team-design-system All issues relating to design system in Extension labels Jun 8, 2023
@SamantaTarun
Copy link

I'd like to work on this issue. Please assign it to me

@georgewrmarshall
Copy link
Contributor Author

Hey @tarunsamanta2k20, thanks for your interest. Please feel free to submit a PR ensuring all acceptance criteria is met and tag me as a reviewer

@SamantaTarun
Copy link

SamantaTarun commented Jun 9, 2023 •
edited
Loading

@georgewrmarshall could you please write over here in simple words what will be changes? I mean which files need to be changed by which component?

@georgewrmarshall
Copy link
Contributor Author

Hey @tarunsamanta2k20, if this issue isn't clear maybe check out this other good first issue and have a look at some of the PRs that have been merged to get an idea of the work involved. #17670

@kremalicious
Copy link
Contributor

Hey @georgewrmarshall, this issue's description could lead to some confusion as the file references show the Box component, yet this issue here talks about the ActionableMessage/BannerAlert component.

So all references in the issue description should be changed like so:

  • ui/components/ui/box/box.js → ui/components/ui/actionable-message/actionable-message.js
  • ui/components/component-library/box/box.tsx → ui/components/component-library/banner-alert/banner-alert.js
  • same goes for the storybook link, which falsely links to Box component

Also, description and acceptance criteria are different:

  • description: "PRs should contain no more than 1 file"
  • acceptance criteria: "Each Pull Request (PR) should include no more than 3 files with BannerALert replacements."

Maybe the first statement was supposed to say no more than 1 component?

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Jun 26, 2023 •
edited
Loading

Excellent spotting @kremalicious! Sloppy issue creation on my part. Apologies and thank you for the suggestions I have updated the ticket 🙏

@pritam1813
Copy link
Contributor

pritam1813 commented Jul 18, 2023 •
edited
Loading

Hey @georgewrmarshall , I am new to open source and I would like to work on this issue (If no one is working on it right now). Can you please assign it to me. Thanks.

@georgewrmarshall
Copy link
Contributor Author

Hi @pritam1813, I would suggest getting the extension and storybook working locally first, then I would suggestion working on this issue #17670

@pritam1813
Copy link
Contributor

pritam1813 commented Jul 19, 2023 •
edited
Loading

Hi @georgewrmarshall , thanks for the suggestion. I have already setup the extension and storybook locally.
I Already replaced ActionableMessage with BannerAlert in one file, it passes jest tests and runs fine on storybook. Ready to submit first PR.
If you don't want me to continue working on this issue , please let me know.

Also, the BannerAlert component currently uses SEVERITIES which is a deprecated const , I can fix that too as a part of this issue #18714

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Jul 19, 2023 •
edited
Loading

Hey @pritam1813, that's great to hear! Please go ahead and submit a PR and tag me as a reviewer. Updating SEVERITIES in BannerAlert would also be great. I think that update could possibly go in a separate PR though. Looking forward to your contributions

This was referenced Jul 20, 2023
Merged
Closed
Merged
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 25, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-import-token
29cd01b
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 25, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-import-token
4a80f56
This was referenced Jul 26, 2023
Closed
Merged
Merged
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 26, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-and-MetaMask#18714-…
5861718 
…Replace-SEVERITIES-with-Severity-in-BannerAlert
brad-decker added a commit to pritam1813/metamask-extension that referenced this issue Jul 26, 2023
@brad-decker
Merge branch 'develop' into part-of-MetaMask#19528-and-MetaMask#18714-…
358f747 
…Replace-SEVERITIES-with-Severity-in-BannerAlert
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 27, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-and-MetaMask#18714-…
57f5ea4 
…Replace-SEVERITIES-with-Severity-in-BannerAlert
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 27, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-and-MetaMask#18714-…
97d868f 
…Replace-SEVERITIES-with-Severity-in-BannerAlert
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Jul 28, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-and-MetaMask#18714-…
660bd18 
…Replace-SEVERITIES-with-Severity-in-BannerAlert
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Aug 1, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-edit-gas-display
f22fe83
This was referenced Aug 6, 2023
Closed
Merged
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Aug 13, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-page-co…
bee4e1f 
…ntainer-content
@pritam1813 pritam1813 mentioned this issue Aug 14, 2023
Merged
8 tasks
@pritam1813
Copy link
Contributor

Hi @georgewrmarshall , please review #20415 , #20417 and #20443 .
Can you please guide me, passing e2e tests on #20417 and #20443 .

georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Aug 14, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-page-co…
3efaac6 
…ntainer-content
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Aug 15, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
6c9451f 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Aug 17, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
3ec12da 
…gested-token
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Aug 21, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
ff0953c 
…gested-token
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Aug 21, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
f5b2770 
…gested-token
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Aug 29, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
80c8659 
…gested-token
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Aug 29, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-send-content
9872a0c
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Sep 3, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-send-content
341e0af
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Sep 7, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-send-content
cc5ca01
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Sep 15, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
9ace2ee 
…gested-token
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Sep 15, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-send-content
4bd8398
@pritam1813 pritam1813 mentioned this issue Sep 15, 2023
Merged
8 tasks
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Sep 15, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
0cb2bd9 
…gested-token
garrettbear pushed a commit that referenced this issue Oct 5, 2023
@pritam1813 @georgewrmarshall
Replaced ActionableMessage in Advanced Tab (#20900)
cffa262 
## Explanation
- This PR fixes part of
#19528
- Replaces instances of `ActionableMessage` with `BannerAlert` in
`ui/pages/settings/advanced-tab/advanced-tab.component.js`
<!--
Thanks for the pull request. Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues, Slack conversations, Zendesk issues, user stories,
etc. reviewers should consult to understand this pull request better?
For instance:

* Fixes #12345
* See: #67890
-->

## Screenshots/Screencaps

<!-- If you're making a change to the UI, make sure to capture a
screenshot or a short video showing off your work! -->

### Before

<!-- How did the UI you changed look before your changes? Drag your
file(s) below this line: -->

![before_success](/~https://github.com/MetaMask/metamask-extension/assets/46365255/624b4da9-0b59-4b83-b564-593b9c1328a8)

![before_danger](/~https://github.com/MetaMask/metamask-extension/assets/46365255/1549b1b7-88e9-4459-b1c1-7d39b2ddfa70)

### After

<!-- How does it look now? Drag your file(s) below this line: -->

![after_success](/~https://github.com/MetaMask/metamask-extension/assets/46365255/41079132-ae02-48e5-afec-98b2fd22925b)

![after_danger](/~https://github.com/MetaMask/metamask-extension/assets/46365255/aeaad47a-9461-4942-bdd1-e9018167c6e5)

## Manual Testing Steps

<!--
How should reviewers and QA manually test your changes? For instance:

- Go to this screen
- Do this
- Then do this
-->
- Create build from this PR
- From Menu go to Settings -> Advanced
- Scroll Down to find **Restore user data** section.
- Try Restoring with proper/corrupted file. 
## Pre-merge author checklist

- [x] I've clearly explained:
  - [x] What problem this PR is solving
  - [x] How this problem was solved
  - [x] How reviewers can test my changes
- [ ] Sufficient automated test coverage has been added

## Pre-merge reviewer checklist

- [ ] Manual testing (e.g. pull and build branch, run in browser, test
code being changed)
- [ ] PR is linked to the appropriate GitHub issue
- [ ] **IF** this PR fixes a bug in the release milestone, add this PR
to the release milestone

If further QA is required (e.g. new feature, complex testing steps,
large refactor), add the `Extension QA Board` label.

In this case, a QA Engineer approval will be be required.

---------

Co-authored-by: georgewrmarshall <george.marshall@consensys.net>
pritam1813 added a commit to pritam1813/metamask-extension that referenced this issue Oct 18, 2023
@pritam1813
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
05e58f3 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Oct 18, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
54f7dac 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Oct 25, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
00fd1a3 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Oct 25, 2023
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-send-content
3c33815
k-g-j pushed a commit that referenced this issue Nov 1, 2023
@pritam1813 @georgewrmarshall @k-g-j
Replaced ActionableMessage in Advanced Tab (#20900)
b2d1cfa 
## Explanation
- This PR fixes part of
#19528
- Replaces instances of `ActionableMessage` with `BannerAlert` in
`ui/pages/settings/advanced-tab/advanced-tab.component.js`
<!--
Thanks for the pull request. Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues, Slack conversations, Zendesk issues, user stories,
etc. reviewers should consult to understand this pull request better?
For instance:

* Fixes #12345
* See: #67890
-->

## Screenshots/Screencaps

<!-- If you're making a change to the UI, make sure to capture a
screenshot or a short video showing off your work! -->

### Before

<!-- How did the UI you changed look before your changes? Drag your
file(s) below this line: -->

![before_success](/~https://github.com/MetaMask/metamask-extension/assets/46365255/624b4da9-0b59-4b83-b564-593b9c1328a8)

![before_danger](/~https://github.com/MetaMask/metamask-extension/assets/46365255/1549b1b7-88e9-4459-b1c1-7d39b2ddfa70)

### After

<!-- How does it look now? Drag your file(s) below this line: -->

![after_success](/~https://github.com/MetaMask/metamask-extension/assets/46365255/41079132-ae02-48e5-afec-98b2fd22925b)

![after_danger](/~https://github.com/MetaMask/metamask-extension/assets/46365255/aeaad47a-9461-4942-bdd1-e9018167c6e5)

## Manual Testing Steps

<!--
How should reviewers and QA manually test your changes? For instance:

- Go to this screen
- Do this
- Then do this
-->
- Create build from this PR
- From Menu go to Settings -> Advanced
- Scroll Down to find **Restore user data** section.
- Try Restoring with proper/corrupted file. 
## Pre-merge author checklist

- [x] I've clearly explained:
  - [x] What problem this PR is solving
  - [x] How this problem was solved
  - [x] How reviewers can test my changes
- [ ] Sufficient automated test coverage has been added

## Pre-merge reviewer checklist

- [ ] Manual testing (e.g. pull and build branch, run in browser, test
code being changed)
- [ ] PR is linked to the appropriate GitHub issue
- [ ] **IF** this PR fixes a bug in the release milestone, add this PR
to the release milestone

If further QA is required (e.g. new feature, complex testing steps,
large refactor), add the `Extension QA Board` label.

In this case, a QA Engineer approval will be be required.

---------

Co-authored-by: georgewrmarshall <george.marshall@consensys.net>
darkwing added a commit to pritam1813/metamask-extension that referenced this issue Dec 4, 2023
@darkwing
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
cfafb8f 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Jan 12, 2024
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
c12179b 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Jan 17, 2024
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
b0e3a79 
…gested-token
georgewrmarshall added a commit to pritam1813/metamask-extension that referenced this issue Jan 20, 2024
@georgewrmarshall
Merge branch 'develop' into part-of-MetaMask#19528-in-confirm-add-sug…
65674cf 
…gested-token
garrettbear pushed a commit that referenced this issue Jan 24, 2024
@pritam1813 @georgewrmarshall @darkwing
Replaced ActionableMessage and Deprecated Button (#20443)
22174a1 
## Explanation

- This PR fixes Part of
#19528 and
#18896
- Replaces `ActionableMessage` with `BannerAlert` in
`ui/pages/confirm-add-suggested-token/confirm-add-suggested-token.js`
- Replaces old `<Button />` component (ui/components/component-library)
with new `<Button />` component
(ui/components/component-library/button/button.js) inside the
`BannerAlert` component.
<!--
Thanks for the pull request. Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues, Slack conversations, Zendesk issues, user stories,
etc. reviewers should consult to understand this pull request better?
For instance:

* Fixes #12345
* See: #67890
-->

## Screenshots/Screencaps

<!-- If you're making a change to the UI, make sure to capture a
screenshot or a short video showing off your work! -->

### Before
**Token With Same Symbol But Different Address (Before)**

![token-with-same-symbol-but-different-address_Before](/~https://github.com/MetaMask/metamask-extension/assets/46365255/cf97ac7f-53e4-409e-ae35-e692a677f282)

**Token With Duplicate Contract Address (Before)**

![Same-address-different-symbol_Before](/~https://github.com/MetaMask/metamask-extension/assets/46365255/35291d3f-4262-44cb-a166-da2358381ba1)

<!-- How did the UI you changed look before your changes? Drag your
file(s) below this line: -->

### After
**Token With Same Symbol But Different Address (After)**

![after_with_diff_addrs](/~https://github.com/MetaMask/metamask-extension/assets/46365255/97f8ed17-8880-45e2-be86-a83f86aedda5)


**Token With Duplicate Contract Address (After)**

![after_with_same_address](/~https://github.com/MetaMask/metamask-extension/assets/46365255/f5d75432-7499-4133-bc08-136acafedc98)


<!-- How does it look now? Drag your file(s) below this line: -->

## Manual Testing Steps
1. Create and Install the new build with this PR.
2. Visit and Connect the Wallet on [Test Dapp
Site](https://metamask.github.io/test-dapp/) .
3. Input token address, symbol and decimal under **_EIP 747_** section
and click **_Add Token_**.
4. Test with a token that is already added to the wallet.
   - Input same address but different token name.
   - Input different contract address and same token name.
<!--
How should reviewers and QA manually test your changes? For instance:

- Go to this screen
- Do this
- Then do this
-->

## Pre-merge author checklist

- [x] I've clearly explained:
  - [x] What problem this PR is solving
  - [x] How this problem was solved
  - [x] How reviewers can test my changes
- [x] Sufficient automated test coverage has been added

## Pre-merge reviewer checklist

- [ ] Manual testing (e.g. pull and build branch, run in browser, test
code being changed)
- [ ] PR is linked to the appropriate GitHub issue
- [ ] **IF** this PR fixes a bug in the release milestone, add this PR
to the release milestone

If further QA is required (e.g. new feature, complex testing steps,
large refactor), add the `Extension QA Board` label.

In this case, a QA Engineer approval will be be required.

---------

Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: David Walsh <davidwalsh83@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers team-design-system All issues relating to design system in Extension
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kremalicious @georgewrmarshall @pritam1813 @SamantaTarun