-
Notifications
You must be signed in to change notification settings - Fork 364
Adding Metaplex Core support #431
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
Adding Metaplex Core support #431
Conversation
@blockiosaurus is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for all the hardwork here Keith! Can you provide some sample accounts & screenshots for me to validate? There's some stuff to nit over, but want to get the big stuff done first. |
Sure! Here's a Core collection The Core UI should have the details you need to confirm but let me know if you need anything else! |
Okay the Asset renders great, but the Core collection's address causes the page to crash. ![]() Can you help me fix this? I want to write tests for this PR like the ones I added here https://github.com/solana-labs/explorer/blob/master/app/components/instruction/lighthouse/__tests__/LighthouseDetailsCard.test.tsx In order to do this, I need some pages that render correctly. Then I can add tests, make sure things work the way we expect, and then provide feedback on code style etc. FWIW on Asset, I think it would be better to render the Core /metadata section as an actual table instead of a JSON view. It's nicer to be able to hyperlink to owner/delegate etc. Wdyt? ![]() |
OK, pushed a fix for parsing collections. |
Tests added. Wen merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great, very clean change. Thanks for the reminder ping on this.
Can you provide a few urls for me to test locally?
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@blockiosaurus Also example tests for integration look like this (lighthouse). The goal is to automatically check expected layout of data so I don't have to remember what each card is supposed to look like |
OK, updated fixing the feedback. I've made some somewhat opinionated decisions on the best way to represent assets (TM and Core) that's very dev-focused, so let me know if you have a different perspective. Collections to test with: Assets: |
Hey @blockiosaurus I'm happy to review this, but you haven't addressed my last comment, nor have you added test, nor have you updated lint to pass CI. Can you please address these items before requesting a review from me? |
Do you mean additional tests beyond the ones I've already added? It's hard to tell what you mean when no other plugin seems to have tests either 😅. |
Closing this PR until the Explorer team creates actual contribution guidelines and is ready to accept external contributions without constantly moving goalposts. |
No description provided.