Replace bitcoin-icons-react with popicons

amitamrutiya commented 8 months ago

Describe the changes you have made in this PR

In this pull request, I replaced the majority of icons with icons from the bitcoin-icons-react library, specifically using popicons icons.

Link this PR to an issue [optional]

Fixes #2920

Type of change

I encountered an issue with some icons where I either couldn't find them in the popicons library, or if I did, there were differences between those icons. Therefore, I didn't completely remove the bitcoin-icons-react library. This can be resolved by adding SVGs in the icons/popicons directory.
I made adjustments to the height and width of some icons to match the specifications in the Figma file and to maintain consistency with the previous icons.

Screenshots of the changes [optional]

Here, I have attached some screenshots highlighting some of the major changes that I made.

These are big changes that happens during migrate to popicons icons.

How has this been tested?

I test all of the changes in before and after changes.

Checklist

[X] Self-review of changed code
[X] Manual testing
[X] Added automated tests where applicable
[X] Update Docs & Guides
For UI-related changes
[X] Darkmode
[X] Responsive layout

Additional Information

This PR may take some significant time to review because you need to compare the before and after changes of all the icons. I understand that merging it in one go is impossible, so please take your time to review it thoroughly. Feel free to provide suggestions for improving the icons, and I will make modifications accordingly.

reneaaron commented 8 months ago

@amitamrutiya2210 Wow, thanks for this PR - this is great! :100:

I encountered an issue with some icons where I either couldn't find them in the popicons library, or if I did, there were differences between those icons. Therefore, I didn't completely remove the bitcoin-icons-react library. This can be resolved by adding SVGs in the icons/popicons directory.

I think we should just try to find icons that somehow fit the purpose, even if they are not exactly the same. Do you want to go ahead and give the remaining ones a try and see if you can find some alternative icons that fit the purpose?

I would definitely want to remove the bitcoin-design icon library with this PR so we don't have 2 libraries in there anymore.

I made adjustments to the height and width of some icons to match the specifications in the Figma file and to maintain consistency with the previous icons.

Okay, I'll have a look at these. I think the sizing of the icons is sometimes quite different to the bitcoin-design icon set and I would rather try to aim for consistency.

amitamrutiya commented 8 months ago

Thanks, @reneaaron, for the appreciation. I would like to work alongside you on this PR. First, I will try to replace existing bitcoin-icons-react icons with popicons icons that fulfill the functionality. If this approach fails, I will create SVG icons in the static/assets/icons/popicons directory and import them from there.

socket-security[bot] commented 8 months ago

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@bitcoin-design/bitcoin-icons-react@0.1.10

View full report↗︎

amitamrutiya commented 8 months ago

@reneaaron,

Now this is ready for review. I have completely removed the bitcoin-icons-react library. I attempted to find all possible solutions for some icons in popicons, but unfortunately, I could not find 8 icons, so I placed SVGs in those locations.

I also replaced some icons that tried to fulfill the purpose of the context, but if they don't seem suitable to you, please let me know and I will make changes accordingly.

Note: Please ensure you check the height and width of the icons. I set the height and width according to my knowledge, but you definitely have a better understanding of the height and width of icons in this project.

socket-security[bot] commented 8 months ago

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

reneaaron commented 8 months ago

@amitamrutiya2210 Amazing work, thank you very much! :100:

I worked through the changes and added some minor adjustments, please have a look and see if these changes make sense to you!

For the popicons missing in the official npm package I asked the author to do a release with the new icons. In the meantime I created a popicons folder where we can maintain the missing icons ourselves.

I would want to keep any SVGs out of the react components and possibly create separate components for them as I think they clutter the code quite a bit and should be reusable from different components.

FYI: Also I sneaked in some minor style fixes where icon colors didn't match the designs.

amitamrutiya commented 8 months ago

@reneaaron, I have reviewed all of your changes and tested them in both light and dark modes. They work quite well. You are correct that moving SVGs out of React components will enhance code readability and maintainability. All of your style changes make sense to me.

reneaaron commented 8 months ago

@SocketSecurity ignore npm/blech32@1.1.2

getAlby / lightning-browser-extension