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

Implement NFTs V2 Arc endpoint #5973

Merged
merged 14 commits into from
Aug 15, 2024
Merged

Implement NFTs V2 Arc endpoint #5973

merged 14 commits into from
Aug 15, 2024

Conversation

walmat
Copy link
Contributor

@walmat walmat commented Jul 31, 2024

Fixes APP-1594

  • revert graphql config changes before merge

What changed (plus any additional context for devs)

The major part of this PR includes consuming a new and faster nftV2 endpoint from Arc that @welps wrote. This cuts down nft fetching time by about 3x on average. There are still improvements to be made to it, but at least with his endpoint in, accounts with large amounts of NFTs should be able to see their collectibles again.

Other minor changes included removing the check or if the NFT section was empty, instead we now show an empty state and allow the user to peek through to the featured mint section if they wish to collect.

We needed to remove the check to remove the NFT section on empty because the NFT endpoint now fetches and sorts from the backend, so each time the user changed the sort method the Collectibles header would disappear.

We also now have a skeleton loader for when the NFT fetch is in-flight and no data is present. This helps let the user know that their collectibles are loading instead of just removing the section altogether.

Screen recordings / screenshots

Screenshot 2024-07-31 at 5 27 45 PM
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-01.at.09.58.17.mp4

What to test

Copy link

linear bot commented Aug 1, 2024

@walmat walmat marked this pull request as ready for review August 1, 2024 14:07
@benisgold benisgold self-requested a review August 8, 2024 20:38
Copy link
Member

@benisgold benisgold left a comment

Choose a reason for hiding this comment

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

looks pretty good, just minor comments. i saw a few places where we're relying on react query's isFetching- just want to double check that that's what we want vs. isLoading. also i noticed in your screenshot that the height of the "Collect Now" button is messed up

src/graphql/config.js Outdated Show resolved Hide resolved
@walmat
Copy link
Contributor Author

walmat commented Aug 9, 2024

looks pretty good, just minor comments. i saw a few places where we're relying on react query's isFetching- just want to double check that that's what we want vs. isLoading. also i noticed in your screenshot that the height of the "Collect Now" button is messed up

Yeah I played around with this a bit and think isLoading would remove the data on pull to referesh whereas isFetching wouldn't. I think that's the behavior we want.

@walmat walmat requested a review from benisgold August 9, 2024 15:49
Copy link
Member

@benisgold benisgold left a comment

Choose a reason for hiding this comment

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

It takes a second or two for the context menu text to switch from one option to the other. It should switch immediately after the user makes the selection even if the data is still loading. not sure what's causing the lag

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-09.at.16.49.06.mp4

@walmat
Copy link
Contributor Author

walmat commented Aug 15, 2024

It takes a second or two for the context menu text to switch from one option to the other. It should switch immediately after the user makes the selection even if the data is still loading. not sure what's causing the lag

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-09.at.16.49.06.mp4

this is fixed now

@walmat walmat requested a review from benisgold August 15, 2024 17:30
@walmat walmat merged commit 091ceac into develop Aug 15, 2024
6 checks passed
@walmat walmat deleted the @matthew/fix-nfts-stuff branch August 15, 2024 20:37
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.

2 participants