Skip to content

Conversation

@phoniks
Copy link
Contributor

@phoniks phoniks commented Oct 26, 2022

No description provided.

Copy link
Contributor

@DanBurton DanBurton left a comment

Choose a reason for hiding this comment

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

filter/sort stuff lgtm but the detail page got messed up.

<Rarity>{nft ? nft.rarity : ''}</Rarity>
</Badge>
<Image src={data.url} />
<Image src={nft ? nft.rarity : ''} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Image src={nft ? nft.rarity : ''} />
<Image src={nft ? nft.url : ''} />

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one thing wrong with the details page but I think there might be more changes needed to fix it.

@DanBurton
Copy link
Contributor

Can you also rebase your branch on latest master?

@DanBurton
Copy link
Contributor

@phoniks I see that you force pushed, which I presume was an attempt at rebasing on latest master, but it's still showing a bunch of merge conflicts. Can you sort out the merge conflicts and get this PR is in a mergeable state?

Copy link
Contributor

@DanBurton DanBurton left a comment

Choose a reason for hiding this comment

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

When running it locally I'm observing that the filters don't work as intended.

  • Only the for sale ones seem to show up regardless of whether I've selected the "for sale" tab or not on top.
  • No matter which face filter I select, it only shows me the ones corresponding to nft.attributes.face == 0
  • Similarly, no matter which glasses filter I select, it only shows me the ones corresponding to nft.attributers.glasses == 1

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.

3 participants