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

Separated NFT grid #195

Merged
merged 4 commits into from
Jun 26, 2022

Conversation

modersohn
Copy link
Collaborator

Closes #193

Wow, that has been quite a ride, lots of new things learnt, but I think I got everything! Cancellation etc. work, directly changing page numbers via the URL etc. all still seem to work just fine!

No visual changes AFAICS.

The grid needs to have a list of AccountNFTSlot, not NonFungibleToken, for it to display the balance - when viewing on AccountDetails. For the collection view, a LINQ transformation of the list is therefore necessary, but I don't think that'll hurt.

Please give it a thorough test drive on dev.

Now that this is separated out, we can tackle #180

* PageNumber with two-way binding because it's a query parameter
  of the contained page
* own OnParametersSetAsync with typical CTS logic
* implements IDispose so it can cancel possibly running OnParameter...
* collections page now keeps a maxPageCount so you don't loose that
  info if you're going back pages
* needed for chip with balance
* collection page transforms list of NonFungibleToken to list
  of AccountNFTSlots, only setting nft field
* now variables like NFTSlots are no longer misleading, because
  they are slots (again)
* only show chip with balance if it's positive
* now that we're only loading the slots and the actual NFTs are
  loaded asynchronously in NFTGrid, it's better to load them before
  transactions, because a) these take long to load and b) they're
  displayed last so should be loaded last
@modersohn modersohn requested a review from fudgebucket27 June 25, 2022 13:19
Copy link
Owner

@fudgebucket27 fudgebucket27 left a comment

Choose a reason for hiding this comment

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

Works great and is much cleaner having it in one component now! Will merge into dev.

@fudgebucket27 fudgebucket27 merged commit 8cefbaa into fudgebucket27:dev Jun 26, 2022
@modersohn modersohn deleted the feature/NFTGridComponent branch June 26, 2022 12:03
@modersohn
Copy link
Collaborator Author

We should wait with merging this into production!

I've just found out that, because the NFTGrid is now a child component, it's OnParameterSetAsync is called way more often, even if parameters actually didn't change. (This is an interesting read BTW https://www.thinktecture.com/blazor/blazor-components-lifecycle-is-not-always-straightforward/)

That is problematic, because every new call to OnParameterSetAsync will cancel the cancellationToken from the previous run. This is visible in the console with "tons" of Request aborted messages and that also means we're cancelling many RestRequests and then immediately do them again. To see that in action, change the page while the NFTs are still loading. I would expect one Request aborted as we're only fetching one at a time - but I see 5 or 6.

@fudgebucket27
Copy link
Owner

Ahhhh I didn't notice that in the console. Thanks for the heads up then.

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