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

Fix nft details in carousel #4413

Merged
merged 7 commits into from
Nov 30, 2022
Merged

Fix nft details in carousel #4413

merged 7 commits into from
Nov 30, 2022

Conversation

shashkovdanil
Copy link
Contributor

@shashkovdanil shashkovdanil commented Nov 25, 2022

Important!

This fix works well, but it's temporary. The backend developers need to figure out why the metadata is empty for some NFTs.

PR Type

Bugfix

Context

Had issue bounty label?

  • Fill up your KSM address: Payout

Screenshot 📸

https://monosnap.com/file/jNQh60q7PLkfXgzjKY3JPLiPF5Zsjb

@shashkovdanil shashkovdanil requested a review from a team as a code owner November 25, 2022 14:36
@shashkovdanil shashkovdanil requested review from roiLeo and removed request for a team November 25, 2022 14:36
@kodabot
Copy link
Collaborator

kodabot commented Nov 25, 2022

SUCCESS @shashkovdanil PR for issue #4169 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Nov 25, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 9a32d0f
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/6384d5bac20c8b0008a12f94
😎 Deploy Preview https://deploy-preview-4413--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yangwao
Copy link
Member

yangwao commented Nov 25, 2022

The backend developers need to figure out why the metadata is empty for some NFTs.

I would check here?

@shashkovdanil
Copy link
Contributor Author

The backend developers need to figure out why the metadata is empty for some NFTs.

I would check here?

Yep, that's it. I would look at it myself, but I'm afraid I don't have enough experience to figure it out on my own in a normal amount of time.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

Here's the issue category breakdown:
Category Count
Complexity 1

Could you find a way to reduce numbers of line for formatNFT method?

@shashkovdanil
Copy link
Contributor Author

@roiLeo think I can, but it will make the code unreadable. GitHub shows that I made many changes because I wrapped data.map in Promise.all(data.map). In fact my changes are:

if (metaImage === null) {
  const cachedMeta = await get(nft.metadata)
  const meta = !isEmpty(cachedMeta)
    ? cachedMeta
    : await fetchNFTMetadata(
        nft,
        getSanitizer(nft.metadata, 'pinata', 'permafrost')
      )
  const imageSanitizer = getSanitizer(meta.image, 'pinata')

  return {
    ...result,
    name: meta.name,
    image: imageSanitizer(meta.image),
    animation_url: sanitizeIpfsUrl(
      meta.animation_url || meta.image,
      'pinata'
    ),
  }
}

utils/carousel.ts Outdated Show resolved Hide resolved
@shashkovdanil
Copy link
Contributor Author

@roiLeo any updates?

@shashkovdanil shashkovdanil requested review from preschian and removed request for roiLeo November 27, 2022 15:09
utils/carousel.ts Outdated Show resolved Hide resolved
@shashkovdanil shashkovdanil requested review from roiLeo and preschian and removed request for preschian and roiLeo November 28, 2022 14:17
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

set in idb new meta val if present

missing setter for idb storage

if (!cachedMeta) {
  set(nft.metadata, meta)
}

@shashkovdanil
Copy link
Contributor Author

set in idb new meta val if present

missing setter for idb storage

if (!cachedMeta) {
  set(nft.metadata, meta)
}

Done

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

rest looks good to me

utils/carousel.ts Show resolved Hide resolved
components/carousel/utils/useCarousel.ts Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Nov 28, 2022

Code Climate has analyzed commit 9a32d0f and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

✅ code lgtm

@roiLeo roiLeo added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Nov 28, 2022
@shashkovdanil
Copy link
Contributor Author

Can it be merged?

@shashkovdanil
Copy link
Contributor Author

cc @roiLeo @preschian

@roiLeo
Copy link
Contributor

roiLeo commented Nov 30, 2022

It need 2nd approval to merge this, Team is currently busy with ongoing event.
In the meantime feel free to take another issue

@yangwao
Copy link
Member

yangwao commented Nov 30, 2022

Team is currently busy with ongoing event.

@yangwao yangwao merged commit 8f5da39 into kodadot:main Nov 30, 2022
@yangwao
Copy link
Member

yangwao commented Nov 30, 2022

pay 60 usd

Team is currently busy with ongoing event.

Yes, we are again in Lisbon.
If you want to come over to Polkadot barcamp today/tmrw, you are welcome!

@yangwao
Copy link
Member

yangwao commented Nov 30, 2022

😍 Perfect, I’ve sent the payout
💵 $60 @ 28.2 USD/KSM ~ 2.128 $KSM
🧗 bXn3ExwLs8rpcpknp9wNqnNVgEYVLnTtFPDrJtaPHYds7Vfjv
🔗 0xeae912266df78e9cd51491cfcb4ff228fedea7428dc1a73610794707b2df8a00

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants