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.

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

UX: Replace all FontAwesome Icons with New Icons #17475

Open
darkwing opened this issue Jan 27, 2023 · 20 comments
Open

UX: Replace all FontAwesome Icons with New Icons #17475

darkwing opened this issue Jan 27, 2023 · 20 comments
Labels
good first issue Good for newcomers team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead ux-enhancement ux-papercuts

Comments

@darkwing
Copy link
Contributor

darkwing commented Jan 27, 2023

Description

In an effort to reduce inconsistencies it would be great to replace all Fontawesome icons with their <Icon> or <ButtonIcon> component equivalent. This is a massive undertaking by itself and creating a single PR would be too large. Instead this issue should be broken up into multiple PRs.

Technical Details

  • Search for Fontawesome icons in the codebase by searching fa fa- and swap it out for the Icon component equivalent

Acceptance Criteria

  • All Fontawesome icons have been replaced with their Icon equivalent
  • PR contains a minimum of 1 file and a maximum of 3 files
  • Add Before / After screenshots to your PR to ensure no visual regressions
  • Reference this issue in the description of your PR
  • All CI tests are passing jest, lint, e2e etc

PRs that don't meet the acceptance criteria may be closed.

References

Icon component documentation
ButtonIcon component documentation

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

@darkwing darkwing added type-bug ux-enhancement ux-papercuts team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead good first issue Good for newcomers and removed type-bug labels Jan 27, 2023
@ankit-jds
Copy link

Can you please assign this issue to me??

@georgewrmarshall
Copy link
Contributor

I think this issue is probably quite large to hit in one PR but if it helps we could isolate the icons we currently have that we could swap out and pick one? This is pretty rough and I could break these up into separate issues but if anyone would like to create a PR to swap out any of these FontAwesome icons with the Icon component and ICON_NAMES object equivalent heres a good visual reference.

We still need to add the ones in red but there should be available replacements for anything that has an ICON_NAMES next to it

Screenshot 2023-02-05 at 9 18 30 PM
Screenshot 2023-02-05 at 9 18 37 PM
Screenshot 2023-02-05 at 9 18 45 PM
Screenshot 2023-02-05 at 9 18 57 PM
Screenshot 2023-02-05 at 9 19 03 PM

@NidhiKJha
Copy link
Member

NidhiKJha commented Feb 7, 2023

Thanks, @georgewrmarshall That's the idea, we are not going to add a single PR to update all the icons instead we will be creating multiple PRs to update each icon and then we are planning to update this issue accordingly.

@spiritanand
Copy link
Contributor

Hey @NidhiKJha and @georgewrmarshall

I would love to help you guys in replacing all the icons (one PR at a time)
Could you direct me to an issue that was created that looks to solve this?

@georgewrmarshall
Copy link
Contributor

@spiritanand tagged you in #17670

@moheet333
Copy link

Hi @darkwing is the issue available to work on? new to open source here can you help me where to start?

@sambhavgupta0705
Copy link

Hey @NidhiKJha and @georgewrmarshall
I would like to contribute to this issue:)

@VedanthB
Copy link

Hey @NidhiKJha and @georgewrmarshall I have raised a PR to replace "fa-info" icons , please review.

@georgewrmarshall
Copy link
Contributor

Hi @VedanthB, Thanks for your contribution but I believe @NidhiKJha has a PR open for this task #17539. If you would like to contribute to the extension code base you could help replace Typography with Text and submit a PR against this issue #17670

@VedanthB
Copy link

Hi @VedanthB, Thanks for your contribution but I believe @NidhiKJha has a PR open for this task #17539. If you would like to contribute to the extension code base you could help replace Typography with Text and submit a PR against this issue #17670

Hey @georgewrmarshall , I see that all the tasks have PRs raised for this issue #17670 but some have review comments that are not addressed , should I pick one of those ?

@georgewrmarshall
Copy link
Contributor

Hey @VedanthB, this issue represents a task that can be broken up into multiple smaller PRs. Do a search for <Typography in the code base and pick a file that's not listed here. Comment on the issue and I can add it to the list that you are going to work on it

Screenshot 2023-03-28 at 1 30 53 PM

@RohanJacob23 RohanJacob23 mentioned this issue Apr 20, 2023
8 tasks
@RohanJacob23
Copy link

Hi @georgewrmarshall,
I have raised a PR to replace "fa-lock" icons, can you please review it.

@shyamtawli
Copy link

Hey, Is this is open?

@georgewrmarshall
Copy link
Contributor

Hey @shyamtawli, yes it is. I've updated the issue description hope that helps

@iamenochlee
Copy link

is this still open? can i work on this? @georgewrmarshall

@georgewrmarshall
Copy link
Contributor

Hey @iamenochlee, yes. The ICON_NAMES object has been deprecated in favour of the IconName enum but the name should be the same. Just the casing the is different

@AdeJulius46
Copy link

Hey @georgewrmarshall is this still opened

@georgewrmarshall
Copy link
Contributor

Hey @AdeJulius46, yes it is. I would suggest starting with a single file PR

@AdeJulius46
Copy link

@georgewrmarshall Single PR file ? can you please expantiate this is my first time working on an open source

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Feb 27, 2024

Hello @AdeJulius46,

Apologies for not being clearer earlier. To facilitate the review process and increase the likelihood of your PRs being merged, it's best to submit PRs that modify only a single file. This approach significantly simplifies the review process for our team.

Before submitting a PR, please ensure you have the extension running locally. This step is crucial as it enables you to provide the necessary before and after screencasts or screenshots, which are invaluable for our review process. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead ux-enhancement ux-papercuts
Projects
None yet
Development

No branches or pull requests