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

Collection Page #30

Merged
37 commits merged into from
Jan 24, 2023
Merged

Collection Page #30

37 commits merged into from
Jan 24, 2023

Conversation

filipmanole
Copy link
Contributor

@filipmanole filipmanole commented Jan 12, 2023

Implementing #22 #11

  • Fetching NFT ids by filtering past events.
  • Fetching metadata in chunks while scrolling.

@filipmanole
Copy link
Contributor Author

Short preview:

image

@ghost ghost self-requested a review January 12, 2023 22:05
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

src/components/NFTCard/NFTCard.scss Outdated Show resolved Hide resolved
src/components/NFTCard/NFTCard.tsx Outdated Show resolved Hide resolved
src/components/NFTCard/NFTCard.tsx Outdated Show resolved Hide resolved
src/components/NFTCard/NFTCard.tsx Outdated Show resolved Hide resolved
src/hooks/useContract.ts Outdated Show resolved Hide resolved
src/pages/CollectionPage/CollectionPage.tsx Outdated Show resolved Hide resolved
src/pages/CollectionPage/CollectionPage.tsx Outdated Show resolved Hide resolved
src/pages/CollectionPage/CollectionPage.tsx Outdated Show resolved Hide resolved
@filipmanole
Copy link
Contributor Author

filipmanole commented Jan 20, 2023

Short video showing the collection page:
https://www.loom.com/share/b1d66dd9755e4123873a121e6797c33b

@filipmanole filipmanole requested a review from a user January 21, 2023 16:09
@filipmanole filipmanole marked this pull request as ready for review January 23, 2023 09:18
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good. Things are working nicely. I added some commits to make it more solid:

  1. Your fetchNFTMetadata was not setup properly. You only have to make it return a promise and then get the responses in a reducer with the pending, fulfilled, and rejected. So I removed all the collection reducers because they are simply not necessary.

  2. You had an await fetchCallback() inside the useInfiniteScroll. You should keep all async logic inside the store. So I added an isLoading to the collection store.

  3. I removed configureCollectionSubscriber. I understand it's confusing that there's also a configureBalancesSubscriber and configureMetadataSubscriber. But those subscribers are there because those stores can not be called from a specific widget. The collection store is connected to the CollectionWidget, so actions can be called from there.

  4. You used a lot of grid in your css. It's only necessary when you're using a grid, like the nfts grid. So I left it there only.

  5. You had a onClick prop on NftCard and used navigate for navigation. You should simply use NavLink without any listeners.

  6. You had classNames like collection-widget__content__subtitle, while technically correct it's better to just use collection-widget__subtitle.

Things that still need work I have made tickets for:

Infinite scroll and loader needs some work still:
#45

IERC1155 support:
#46

Price of token:
#47

Error handling:
#48

Add support for irregular token id numbers
#49

@ghost ghost merged commit cb06d66 into main Jan 24, 2023
@ghost ghost deleted the feature/collection-page branch January 24, 2023 17:57
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Research how to get all the different tokens in a ERC-721
2 participants