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

Use web3icons #143

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Use web3icons #143

merged 3 commits into from
Aug 20, 2024

Conversation

0xa3k5
Copy link
Contributor

@0xa3k5 0xa3k5 commented Aug 18, 2024

Description

uses NetworkIcon from web3icons in the NetworksDropdown component. The NetworkIcon component relies on a chain's id (chainId) so it is future proof, most of the chains in the viem is supported in the library, and library is growing.

I've specifically did not delete the .svg images under the public folder, from what I can tell, they are used in OG image generation, I'm not 100% sure if there are any other place that uses the .icon properties that relies on the .svg files. Feel free to expand on the PR to include them if you think it's necessary.

https://share.cleanshot.com/G3p8vj1s

Additional Information

Related Issues

Closes #117

Your ENS/address: a3k5.eth

Copy link

vercel bot commented Aug 18, 2024

@0xa3k5 is attempting to deploy a commit to the BuidlGuidl Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Aug 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
abi-ninja-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 20, 2024 9:32am

yarn.lock Outdated Show resolved Hide resolved
Copy link
Member

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Looks good to me! The only concern for me is the library size

image

But makes sense to have it large since we have svg images in their. Also once advantage is that when people add chain from "other chain" they get the image out of box. Also I don't see any delay in website initial load (maybe next optimising thing at build time)

@0xa3k5
Copy link
Contributor Author

0xa3k5 commented Aug 19, 2024

@technophile-04 the inquirer is used in the @web3icons/utils which is a direct dependency of @web3icons/react I'm working on some improvements which should help reduce the unpacked size and also reduce the dependencies of the package. Thanks for the review, I really like abi.ninja, joined the Discord server, lmk if I can contribute any other way

Copy link
Member

@portdeveloper portdeveloper left a comment

Choose a reason for hiding this comment

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

There is a minor layout shift but that could be because of my connection!!
we can fix that later if it becomes a problem

lgtm!

@0xa3k5
Copy link
Contributor Author

0xa3k5 commented Aug 19, 2024

hey @technophile-04 & @portdeveloper I've just released a new version to the @web3icons/react that reduces it's dependencies (mainly inquirer) as well as introduces some more network icons, thought I'd commit to the PR while it's still open.

@portdeveloper
Copy link
Member

hey @technophile-04 & @portdeveloper I've just released a new version to the @web3icons/react that reduces it's dependencies (mainly inquirer) as well as introduces some more network icons, thought I'd commit to the PR while it's still open.

Thank you for the hard work! I think what you're doing with web3icons is amazing! Hyped to get this merged. 🫡

@technophile-04
Copy link
Member

hey @technophile-04 & @portdeveloper I've just released a new version to the @web3icons/react that reduces it's dependencies (mainly inquirer) as well as introduces some more network icons, thought I'd commit to the PR while it's still open.

Tysm @0xa3k5! For so quick updates!

@technophile-04 technophile-04 merged commit dd901af into BuidlGuidl:main Aug 20, 2024
3 of 4 checks passed
@technophile-04
Copy link
Member

Just curious can't we use the same component for og images? Looking at

<img tw="mr-4 h-16" src={networkIcon} alt={networkName} />

It seems that we are using jsx here right?

@portdeveloper
Copy link
Member

Just curious can't we use the same component for og images? Looking at

<img tw="mr-4 h-16" src={networkIcon} alt={networkName} />

It seems that we are using jsx here right?

I have just tested it and It does not work.

This could be well due to how image generation works in vercel/og, since it does not have access to the default nodejs runtime.

@0xa3k5
Copy link
Contributor Author

0xa3k5 commented Aug 20, 2024

The <NetworkIcon /> essentially renders a <svg> or <img> idk why it doesn't work, but I'm not very well educated about vercel/og is there a way for me to quickly test og generation? @portdeveloper

@portdeveloper
Copy link
Member

portdeveloper commented Aug 21, 2024

The <NetworkIcon /> essentially renders a <svg> or <img> idk why it doesn't work, but I'm not very well educated about vercel/og is there a way for me to quickly test og generation? @portdeveloper

Since you have abi.ninja on your computer I suggest that you go to:
image

Because this is just vercel/og, anything you put here should work:
image

You can do a yarn start

and check this route as an example:
http://localhost:3000/api/og?contractAddress=0xde30da39c46104798bb5aa3fe8b9e0e1f348163f

If you haven't changed anything you will see:
image

From this stage you can modify the returned ImageResponse.

If you need something simple, maybe this could work better for you:
https://og-playground.vercel.app/

@0xa3k5
Copy link
Contributor Author

0xa3k5 commented Aug 21, 2024

oh tyvm @portdeveloper I'm seeing what the issue is: The <NetworkIcon /> is a client-side component, it uses some hooks and dynamically imports the icons so the bundle size doesn't get bloated. The vercel/og however, works on the server so it can not resolve the NetworkIcon properyl. The @web3icons/core package serves svgs but it might be way overkill to add a dependency. I'm tinkering with a few ideas as a workaround that would not increase the dependencies. I'll let you know if I can figure out something

@portdeveloper
Copy link
Member

@0xa3k5 Thanks for putting in the hard work and dissecting the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra chains logos
3 participants