-
Notifications
You must be signed in to change notification settings - Fork 74
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
Dynamic Unfurl while sharing link #90
Conversation
Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Seems good to me! I think this might be a good option until we have an updated version (which I think we should do at some point, but I don't think is super urgent). I let other review this too |
BTW, this also exists in NextJS (https://nextjs.org/docs/app/api-reference/file-conventions/metadata/opengraph-image#generate-images-using-code-js-ts-tsx) You can create a dynamic OG image with But maybe only in NextJS 14 + App router? |
Yes, I think you are right, I couldn't find the same thing for pages directory. |
Tysm @portdeveloper, just tested it out, and works great !! To test, you should test it locally because if you open deployment URL from vercel it won't work, since we have hardcode https://abi.ninja domain their in MetaHeader Here are the tests : http://localhost:3000/api/og?contractAddress=0x50c5725949A6F0c72E6C4a641F24049A917DB0Cb&network=8453 http://localhost:3000/api/og?contractAddress=0x66496d0d93A77d6360d083f0D80AAEDbd96Dc84f&network=8453 http://localhost:3000/api/og?contractAddress=0x5AbB833336905dE17d13B9615eA3265c18aBb725&network=10 (Not an ERC 20 token contract so won't display contract name) http://localhost:3000/api/og?contractAddress=0xda10009cbd5d07dd0cecc66161fc93d7c9000da1&network=10 http://localhost:3000/api/og?contractAddress=0x07380A04eD702B668DBb7ea38163CcE5097bbE1E&network=1 Just noticed a bug, not related to this PR but also affects this PR is how we are getting Contract Names. The logic which we have in place for both this PR and [networks].tsx page, doesn't work for every contract since "name" function is not exposed by every contract (I initially thought whenever you deploy the contract it creates a getter name function automatically which returns ContractName) ^ But this is not the case, we felt the above logic was working fine but we were mostly testing contracts which were using ERC20 interface / other similar interfaces who exposes "name" function. For example checkout https://abi.ninja/0x9009d498404d32bd8C99D218D31B71b490f2521A/11155111 <= this is SE-2 "YourContract" deployed on sepolia and see it doesn't able to get contract name and fallback's to just saying "Contract". Not sure what's the best approach is to get ContractName. |
@Pabl0cks I think that is an awesome idea! I am in the favor of option 2. About the error, I think it should be fixed thanks to @technophile-04 's patching! check out this link for example |
Yup option 2. looks good, I think we should update it, Thanks @Pabl0cks 🙌
Yup checkout out scaffold-eth/scaffold-eth-2#706 (comment) For testing you could hit (our latest deployment) : https://abi-ninja-v2-3hd2rxxjs-buidlguidldao.vercel.app/api/og?contractAddress=0xde30da39c46104798bb5aa3fe8b9e0e1f348163f (we should hit /api/og? ) for now for testing, hitting https://abi-ninja-v2-3hd2rxxjs-buidlguidldao.vercel.app/0xde30da39c46104798bb5aa3fe8b9e0e1f348163f/1 won't work for now since we have hardcoded |
This is looking great!! Thanks all. Could we add the chain logo next to the chain name? It should be straightforward since we have them for the homepage selector. |
Added it, changed background to look better with current chain icon image assets. We probably should change the image assets to SVGs with no background to fix both problems in a future PR (or current if it looks too bad for you!). Here are a few examples with the same contract and manually changing network:
|
That looks amazing! @Pabl0cks Updated the svg images for chains: arbitrum, base, scroll, zksync: I think this change is minor so we can include it here. |
This is looking great !!!! Tysm @portdeveloper and @Pabl0cks , Merging this 🙌 !! |
Description
Solves #89, with patch to
@noble/hashes
as it was mentioned in wevm/viem#368The patch makes sense but not 100% sure.
Incase we plan to merge this, we should remove this when we update wagmi / viem version + we may need to update nextjs version too (not app router but just next version)...(I think we are already planning to update versions, right?)
Or else maybe we can wait until we have wagmi / viem version upgraded and then this should work out of box without patch
Also lol we should add: Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com> while doing squash and merge I just copy pasted his code from #90 while tinkering.