Adding Metaplex Core support#431
Adding Metaplex Core support#431blockiosaurus wants to merge 14 commits intosolana-foundation:masterfrom
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 |
ngundotra
left a comment
ngundotra
left a comment
There was a problem hiding this comment.
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: |
|
For the collections - wouldn't it be better to just fix the incorrect compressed NFT tagging here? I think those overview fields are more useful than what is shown here, which looks like default card: Normal assets look great. |
|
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. |
2 participants
No description provided.