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

feat: add page for individual nft serials #692

Merged
merged 7 commits into from
Oct 12, 2023
Merged

feat: add page for individual nft serials #692

merged 7 commits into from
Oct 12, 2023

Conversation

Sheng-Long
Copy link
Contributor

Description:
Add page for individual nft serial details (including recent transactions)

Related issue(s):

Fixes #

Notes for reviewer:
Not sure how much token information should be retained/added for this

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@github-actions
Copy link

github-actions bot commented Sep 12, 2023

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 89.51% 30964 / 34592
🔵 Statements 89.51% 30964 / 34592
🔵 Functions 70.27% 768 / 1093
🔵 Branches 84.15% 3551 / 4220
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/router.ts 90.18% 40% 62.5% 90.18% 109-112, 217-220, 255, 271-272, 274-275, 277-278, 280-281, 283-284, 286-287, 289-290, 292-293, 295-296, 298-299, 301-302, 307, 329-330, 364, 373-374
src/components/token/NftHolderTable.vue 90.6% 100% 33.33% 90.6% 113-126
src/components/transaction/NftTransactionAnalyzer.ts 23.48% 100% 0% 23.48% 32-132
src/components/transaction/NftTransactionSummary.vue 74.6% 100% 0% 74.6% 87-118
src/components/transaction/NftTransactionTable.vue 76.16% 100% 0% 76.16% 130-170
src/components/transaction/NftTransactionTableController.ts 21.47% 100% 0% 21.47% 36-163
src/components/transaction/TransactionFilterSelect.vue 96% 90% 75% 96% 86-89
src/components/transfer_graphs/NftDetailsTransferGraph.vue 81.96% 100% 0% 81.96% 133-165
src/components/transfer_graphs/NftTransferGraphSection.vue 85.5% 100% 0% 85.5% 58-67
src/components/values/BlobValue.vue 89.75% 73.52% 100% 89.75% 30, 35, 94-99, 106-110, 127-128, 140-141
src/pages/NftDetails.vue 66.07% 100% 0% 66.07% 210-315, 318-326
src/schemas/HederaSchemas.ts 97.85% 64.28% 100% 97.85% 296-297, 648, 762-763, 765-766, 782-783, 785-792
src/utils/RouteManager.ts 75.85% 97.1% 69.09% 75.85% 64, 83-86, 156-165, 168-177, 195-196, 215-224, 231-234, 237-238, 259-268, 271-275, 278-287, 298-307, 318-327, 338-347, 358-367, 374-379, 382-383, 394-395, 434-435
src/utils/cache/NftBySerialCache.ts 60.65% 100% 33.33% 60.65% 35-50, 53-60
Generated in workflow #508

@Sheng-Long Sheng-Long changed the title WIP: feat: add page for individual nft serials feat: add page for individual nft serials Sep 19, 2023
@Sheng-Long Sheng-Long force-pushed the 00012-nft-page branch 8 times, most recently from a28b7f5 to 2bf2e55 Compare September 21, 2023 17:50
@Sheng-Long Sheng-Long force-pushed the 00012-nft-page branch 19 times, most recently from d9ebede to dd3e712 Compare October 3, 2023 20:02
@Sheng-Long Sheng-Long force-pushed the 00012-nft-page branch 8 times, most recently from b339408 to 3b485a1 Compare October 4, 2023 17:20
@ericleponner ericleponner self-requested a review October 5, 2023 13:55
@Sheng-Long Sheng-Long force-pushed the 00012-nft-page branch 2 times, most recently from 9ad2bd8 to 2fdf2d6 Compare October 6, 2023 21:31
@svienot
Copy link
Collaborator

svienot commented Oct 11, 2023

I played with the current stage of the implementation. Nice work.
Could you make the following adjustments:

  • Recent Transactions Filter: the available types should be just those 7:
    {TOKENMINT, CRYPTOAPPROVEALLOWANCE, CRYPTODELETEALLOWANCE, CRYPTOTRANSFER, TOKENWIPE, TOKENBURN, TOKENDELETION}
    (we have checked this explicitly with Steven Sheehy, lead of mirror-node)
  • Do not display properties when they are null (such as Account ID, Delegating Spender, Spender ID)
    We used to display a lot of "None" values, but we now try to move away from that and densify the contents as it gets more substantial
  • Nice to have: when the metadata property is an IPFS hash, we could display a link (to the public IPFS gateway):
    e.g. if the metadata is: ipfs://bafkreigds272du3nvdaywx4tgebkcw4uegxizku7cyhk3z6bhhwbnxsmwi
    display a link to: https://ipfs.io/ipfs/bafkreigds272du3nvdaywx4tgebkcw4uegxizku7cyhk3z6bhhwbnxsmwi

@svienot
Copy link
Collaborator

svienot commented Oct 12, 2023

I suggest you adjust the failing TokenNavigation e2e test by replacing the sequence which looks for the now disappeared anchor inside the NFT Holder Table by something like this:

        cy.get('#nft-holder-table')
            .find('tbody tr')
            .should('be.visible')
            .should('have.length.at.least', 2)
            .eq(0)
            .find('td')
            .eq(0)
            .click()
            .then(($id) => {
                // cy.log('Selected account Id: ' + $id.text())
                cy.url().should('include', '/mainnet/token/' + nftId + '/' + $id.text())
                cy.contains('Serial Number ' + $id.text())
            })

and perhaps add a few assertions on the content of the page, such that we have at least a bit of e2e coverage for this brand new Serial Details page.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 12 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.17) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

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