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

Replace bitcoin-icons-react with popicons #3072

Merged
merged 18 commits into from
Mar 14, 2024

Conversation

amitamrutiya
Copy link
Contributor

@amitamrutiya amitamrutiya commented Mar 9, 2024

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.

image
image
image

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

  • Self-review of changed code
  • Manual testing
  • Added automated tests where applicable
  • Update Docs & Guides
  • For UI-related changes
  • Darkmode
  • 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.

Signed-off-by: amitamrutiya2210 <amitamrutiya2210@gmail.com>
@amitamrutiya amitamrutiya marked this pull request as ready for review March 9, 2024 06:12
@reneaaron
Copy link
Contributor

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

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
Copy link
Contributor Author

amitamrutiya commented Mar 11, 2024

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.

Signed-off-by: amitamrutiya2210 <amitamrutiya2210@gmail.com>
Signed-off-by: amitamrutiya2210 <amitamrutiya2210@gmail.com>

This comment was marked as outdated.

@amitamrutiya
Copy link
Contributor Author

@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.

@reneaaron reneaaron added the after-next-release Waiting for the next release (e.g. translations) label Mar 13, 2024

This comment was marked as resolved.

@reneaaron
Copy link
Contributor

@amitamrutiya2210 Amazing work, thank you very much! 💯

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
Copy link
Contributor Author

@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
Copy link
Contributor

@SocketSecurity ignore npm/blech32@1.1.2

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@reneaaron reneaaron merged commit c6cef19 into getAlby:master Mar 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-next-release Waiting for the next release (e.g. translations)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change iconset to Popicons
2 participants