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

fix: collection nft error #220

Merged
merged 6 commits into from
Aug 9, 2022
Merged

fix: collection nft error #220

merged 6 commits into from
Aug 9, 2022

Conversation

aspnxdd
Copy link
Contributor

@aspnxdd aspnxdd commented Aug 9, 2022 •
edited
Loading

Fixes #218

Changes

  • This error fixes the issue described in [Bug]: Error fetching CM config #218 . Fetching the NFTs threw an error which was set to the setError state, then the error hid the cm config. If there is a problem fetching the NFTs it should not affect the CM config.
  • Also changed how the collection NFT is rendered. Having 2 states for the collection NFT is a good idea (boolean and data itself), sometimes the NFT collection exists but it is empty, I added some fallback texts to show an empty image and hide the solscan link in case it exists but it is empty.
  • Fixed solscan link in NftCard, an extra ? was causing issues in the url.
  • Stop infinite loading if no nfts are found.

Checks

  • Have you catchup your branch with the latest state in base?
  • Have you lint your code locally prior to submission?
  • Have you reviewed your proposed changes and removed debris?
  • Have you successfully ran due tests with your changes?

@boxfish-bot boxfish-bot temporarily deployed to Preview August 9, 2022 10:30 Inactive
@boxfish-bot boxfish-bot temporarily deployed to Preview August 9, 2022 10:31 Inactive
@aspnxdd aspnxdd requested a review from cpl121 August 9, 2022 10:32
cpl121
cpl121 requested changes Aug 9, 2022
View reviewed changes
Copy link
Member

@cpl121 cpl121 left a comment

Choose a reason for hiding this comment

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

Nice job!!
Just 1 comment, if the user has not minted any nft, the loading is infinite, so it is impossible to mint

@aspnxdd aspnxdd requested a review from cpl121 August 9, 2022 12:49
@boxfish-bot boxfish-bot temporarily deployed to Preview August 9, 2022 12:53 Inactive
@boxfish-bot
Copy link
Member

This pull request has been deployed to Vercel.

Latest commit: 9c60235
✅ Preview: https://candy-machine-dashboard-kscrvhfmc-boxfish-studio.vercel.app
🔍 Inspect: https://vercel.com/boxfish-studio/candy-machine-dashboard/3Vb8Eg2yzdRZDMR7EqLcpYZUX5CP

View Workflow Logs

cpl121
cpl121 approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@cpl121 cpl121 left a comment

Choose a reason for hiding this comment

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

Perfect!

@agus-xyz agus-xyz merged commit d456e32 into main Aug 9, 2022
@agus-xyz agus-xyz deleted the fix/collection-nft-error branch August 9, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpl121 cpl121 cpl121 approved these changes

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

Successfully merging this pull request may close these issues.

[Bug]: Error fetching CM config
4 participants
@aspnxdd @boxfish-bot @cpl121 @agus-xyz