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.

Sign up for GitHub

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

Jump to bottom

NFTs page #7144

Merged
merged 13 commits into from
Mar 15, 2022
Merged

NFTs page #7144

merged 13 commits into from
Mar 15, 2022

Conversation

jsidorenko
Copy link
Contributor

A new page where we could see all the NFT collections exist as well as collections + items that the connected account has.

NFTs page:
image

My NFTs:
image

jacogr
jacogr reviewed Mar 11, 2022
View reviewed changes
Copy link
Member

@jacogr jacogr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Brilliant, this has been outstanding for a very, very long time.

Closes #5397

Will give it a run-through.

@jsidorenko
Copy link
Contributor Author

Hi @jacogr, I've moved the ipfs related logic into a separate hook and now the tests started to fail because of that import: import isIPFS from 'is-ipfs';.
Could you suggest pls how to solve this? Do I need to link that library somewhere besides the hooks/package.json?

@jacogr
Copy link
Member

jacogr commented Mar 15, 2022

The reason the tests are failing is because it uses TextEncoder - that is not available on Jest environments.

jacogr
jacogr reviewed Mar 15, 2022
View reviewed changes
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

This looks really good. Some s/-assets/-nfts/ comments and then some future work comments. But it really looks spot-on.

packages/page-nfts/src/AccountItems/Item.tsx Outdated Show resolved Hide resolved
packages/page-nfts/src/AccountItems/Item.tsx Outdated Show resolved Hide resolved
packages/page-nfts/src/AccountItems/Item.tsx

function Item ({ className, collectionName, value: { account, id, ipfsData } }: Props): React.ReactElement<Props> {
const name = ipfsData?.name || collectionName;
const imageLink = ipfsData?.image ? `https://ipfs.io/ipfs/${ipfsData.image}` : '';
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now, however have has issues in the past where we blew past the ipfs.io limit - so I guess it would make sense to add something where we iterate through different IPFS gateways in the future. (As a follow-up). Something like useIpfsLink(hash) and then it randomly selects one from a list of known gateways and returns it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That gateway is extremely slow, if we decide to display the images on that page - it would be better to create a simple proxy/CDN service to cache them. And yes, we would need to make that gateway's URL configurable

packages/page-nfts/src/AccountItems/Transfer.tsx Outdated Show resolved Hide resolved
packages/page-nfts/src/AccountItems/Transfer.tsx Outdated Show resolved Hide resolved
packages/page-nfts/src/translate.ts Outdated Show resolved Hide resolved
packages/page-nfts/src/translate.ts Outdated Show resolved Hide resolved
packages/page-nfts/src/types.ts Outdated Show resolved Hide resolved
packages/page-nfts/src/useCollectionIds.ts Outdated Show resolved Hide resolved
packages/page-nfts/src/useCollectionInfos.ts Outdated Show resolved Hide resolved
@jsidorenko
Copy link
Contributor Author

The reason the tests are failing is because it uses TextEncoder - that is not available on Jest environments.

Any ideas on how to solve this? I tried to add this polyfill to jest/jest-setup.ts, but it breaks the other things :(

import util from 'util';
if (typeof TextEncoder === 'undefined') {
  global.TextEncoder = util.TextEncoder;
}

@jacogr
Copy link
Member

jacogr commented Mar 15, 2022

Doing this in that test or specific hook should solve it -

import '@polkadot/x-textencoder/shim';

It just exposes the global if not available.

@jsidorenko
Copy link
Contributor Author

Yay, you've saved my day!

jacogr
jacogr approved these changes Mar 15, 2022
View reviewed changes
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Thank you!

@jacogr jacogr merged commit 14c0cb6 into polkadot-js:master Mar 15, 2022
This was referenced Mar 17, 2022
Closed
Closed
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jacogr jacogr jacogr approved these changes

@shawntabrizi shawntabrizi Awaiting requested review from shawntabrizi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jsidorenko @jacogr @polkadot-js-bot