search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.03k stars 4.91k forks source link

Replace deprecated ActionableMessage component with new BannerAlert component #19528

Open georgewrmarshall opened 1 year ago

georgewrmarshall commented 1 year ago

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

Acceptance Criteria

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

SamantaTarun commented 1 year ago

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

georgewrmarshall commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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. https://github.com/MetaMask/metamask-extension/issues/17670

kremalicious commented 1 year ago

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:

Also, description and acceptance criteria are different:

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

georgewrmarshall commented 1 year ago

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

pritam1813 commented 1 year ago

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 commented 1 year ago

Hi @pritam1813, I would suggest getting the extension and storybook working locally first, then I would suggestion working on this issue https://github.com/MetaMask/metamask-extension/issues/17670

pritam1813 commented 1 year ago

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 commented 1 year ago

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

pritam1813 commented 1 year ago

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