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

Adding Medias view to MetadataViews.NFTView #123

Closed
bymi15 opened this issue Sep 1, 2022 · 16 comments
Closed

Adding Medias view to MetadataViews.NFTView #123

bymi15 opened this issue Sep 1, 2022 · 16 comments

Comments

@bymi15
Copy link

bymi15 commented Sep 1, 2022

Issue To Be Solved

Are there any reasons why the Medias view is not included in the MetadataViews.NFTView?
Can this be included as there are quite a few NFT collections that use video files rather than images and this can only be included in the Medias view.

@bshahid331
Copy link
Contributor

I don't see why not, we should be able to add it

@bjartek
Copy link
Contributor

bjartek commented Sep 8, 2022

We cannot add state to this struct as it is already created, then you would need to have a new version of the view with a new name.

@bshahid331
Copy link
Contributor

Maybe we can call it v2?

@bymi15
Copy link
Author

bymi15 commented Sep 8, 2022

I’m guessing that’s due to the limitation of flow contract updatability.
In that case, NFTViewV2 sounds like the way to go.
Maybe there should be some further discussions on other areas of improvements before deciding what needs to be added to V2.

@bshahid331
Copy link
Contributor

Agreed we can make a broader v2 core views issue to discuss what we think should be added

@joshuahannan
Copy link
Member

@bymi15 Can you open a PR with this update? That would be a good place to have this discussion

@bymi15
Copy link
Author

bymi15 commented Sep 14, 2022

@bymi15 Can you open a PR with this update? That would be a good place to have this discussion

Thanks for your response @joshuahannan
I have just raised a PR as you've suggested.

@PeterBenc
Copy link

IMO this is quite an important issue, as far I understand it, the only source of Media for a single nft is the thumbnail field which isn't of type Media (honestly its pretty strange that colleciton-specific data had media files but nfts don't). I was hoping we could migrate from alchemy to nft catalog but since all I have is a thumbnail of which I don't even know what mimeType it is I really can't :(

Pls correct me if I am wrong but seems like as long nft media goes, the thumbnail field is the best I can do right now.

@bymi15
Copy link
Author

bymi15 commented Oct 10, 2022

@PeterBenc I think the thumbnail field was intended more for small-medium sized images that can be displayed on websites.
Totally agree that there should be a field that allows one or more media files (i.e. the medias view should be included in the core NFTView)

@joshuahannan
Copy link
Member

As we said in the PR, the NFTView is not a requirement, it is simply a utility view that combines all other views. If you want Medias to be included in a whole view, you can simply define a view in your script that combines Medias with NFTView and return that. I don't think we're at the point yet where we want to define a new NFTView. I'm tempted to just close this issue

@bymi15
Copy link
Author

bymi15 commented Oct 10, 2022

The problem is that this is all tied in with NFTCatalog which uses NFTView: https://github.com/dapperlabs/nft-catalog/blob/main/cadence/contracts/NFTRetrieval.cdc#L99

Perhaps this could be raised in NFTCatalog - so they can support other metadata views such as Medias, then we could probably close this one.

@bshahid331
Copy link
Contributor

That contract just has just helpful shared functions, they are not mutating any state. I can update those to include Medias but you should be able to do the same from within a script if you need to or any other contract for that matter

@bymi15
Copy link
Author

bymi15 commented Oct 10, 2022

@bshahid331 yeah that's true but I think it would be nice to include other views in getNFTViewsFromCap. Otherwise, we would have to write our own custom script to get the other views such as Medias and getNFTViewsFromCap would not be used at all.

@bshahid331
Copy link
Contributor

bshahid331 commented Oct 10, 2022

yep I can update it but you should be able to unblock yourself if needed

@PeterBenc
Copy link

Thanks for the discussion, I dug a bit deeper and realized I was missing some context in order to comprehend this fully, So I found out I should be able to get other media files and not rely on the thumbnail filed alone, but as @bymi15 said, I ll need to write my own version of getNFTViewsFromCap. Which is fine I guess. Not sure why Medias were excluded from the NFTView which lead us all here 😄 but I suppose this issue is resolved (at least for me). 👍

@bymi15
Copy link
Author

bymi15 commented Oct 11, 2022

@PeterBenc
Great. I’m glad it’s all sorted out for you.
So i guess i’m going to close this issue since there shouldn’t be any blockers due to this.

@bymi15 bymi15 closed this as completed Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants