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

Show Stake Claim NFT details #956

Conversation

GhenadieVP
Copy link
Contributor

Display the stake claim nft details

Demo

RPReplay_Final1702372059.MP4

@@ -189,12 +189,13 @@ extension NonFungibleTokenDetails.ViewState.TokenDetails {
}

extension OnLedgerEntity.NonFungibleToken.NFTData {
private static let standardFields: [OnLedgerEntity.NonFungibleToken.NFTData.StandardField] = [.name, .description, .keyImageURL]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used for now to show the claim_epoch and claim_amount as arbitrary fields. It needs to be clarified if we should display those two fields in a different way in NFT details.

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 a good idea, but for most, if not all, standard fields we will want to map the key to a localised string key, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Standard fields do have a predefined way of being displayed - like the name is shown as the sheet title, the keyImageURL is shown as an image. We don't actually display the name of the field, we do show its content in some predefined way.
But true, if there is ever the case of showing the name of the standard field, it would be localised.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, for those ones, but this PR makes me think that there might be fields that should be displayed as labeled values. I suppose actually it would make more sense to separate that concept from the standard field concept, and just have something like fieldNameMapping: [OnLedgerEntity.NonFungibleToken.NFTData.AnyKey: LocalizedStringKey]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ArbitraryFields are the ones that are named, but not localised. It was introduced this with my previous PR.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

LGTM!

@GhenadieVP GhenadieVP merged commit 5da1120 into main Dec 13, 2023
6 checks passed
@GhenadieVP GhenadieVP deleted the fix/ABW-2619-i-os-1-2-0-stake-claim-nft-screen-not-accessible-when-clicking-ready-to-claim-unstaking branch December 13, 2023 09:47
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
Development

Successfully merging this pull request may close these issues.

3 participants