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

[ABW-3569, ABW-3586] Asset transfer & Transaction review Presenting section UI updates #1214

Merged
merged 13 commits into from
Jul 17, 2024

Conversation

danvleju-rdx
Copy link
Contributor

@danvleju-rdx danvleju-rdx commented Jul 16, 2024

Jira tickets: ABW-3569, ABW-3586

Description

  • Updates the Presenting section of transaction review:

    • Fungible badges will display the icon, name, and quantity.
    • NFT badges will show the resource icon, resource name, NFT name, or local NFT ID, in that order if available.
  • Updates the resource appearance in Asset Transfer screen

Notes

This PR does not cover all the changes that need to be made regarding asset views. More details can be found in the ticket descriptions.

How to test

To test NFT badges from Presenting section stokenet gumball club can be used.

  1. Connect to gumball club
  2. Buy the GC membership card
  3. Then buy gumballs on the account that owns that GC card. You will get 50% discount and the gumballs and you will be able to interact with the badge.

Currently, there is no way to test fungible badges. I have attached a screenshot below showcasing the UI with hardcoded values.

Screenshot

Screen 1 Before Screen 1 After
IMG_0023 IMG_0024

Video

RPReplay_Final1721117531.MP4

@danvleju-rdx danvleju-rdx changed the title Abw 3569 transaction review presenting badges [ABW-3569, ABW-3586] Asset transfer & Transaction review Presenting section UI updates Jul 16, 2024
Copy link
Contributor

@matiasbzurovski matiasbzurovski left a comment

Choose a reason for hiding this comment

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

works good, some code suggestions

}
.padding(.bottom, .medium3)

if proof.id != viewStore.proofs.last?.id {
if proof.id == viewStore.proofs.last?.id {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid this check by just adding a VStack with the ForEach inside and then adding the Separator. Also, instead of setting the .padding(.bottom, .medium3) to each item, we can set it as the VStack spacing and then apply it to the VStack.

VStack(spacing: .medium3) {
	ForEach(viewStore.proofs) { proof in
		ResourceBalanceView(proof.resourceBalance.viewState, appearance: .compact) {
			viewStore.send(.proofTapped(proof))
		}
	}
	Separator()
}
.padding(.bottom, .medium3)

@@ -783,8 +812,8 @@ extension TransactionReview {

extension TransactionReview {
public struct ProofEntity: Sendable, Identifiable, Hashable {
public let id: ResourceAddress
public let metadata: OnLedgerEntity.Metadata
public var id: ResourceBalance { resourceBalance }
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we check a simpler identifier (such as a String) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure if we could use ResourceAddress since there could be multiple NFTs with the same resource address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we cannot use ResourceAddress. However, using ResourceBalance as the id could be quite expensive I guess on long lists.

We are already doing it on places with more elements, so I wouldn't say you need to fix it for this (that won't have too many NFTs as in the Assets View).

Comment on lines 438 to 456
case let .fungible(fungible):
state.destination = .fungibleTokenDetails(.init(
resourceAddress: resource.resourceAddress,
resource: .success(resource),
ownedFungibleResource: .init(
resourceAddress: resource.resourceAddress,
atLedgerState: resource.atLedgerState,
amount: fungible.amount,
metadata: resource.metadata
),
isXRD: resource.resourceAddress.isXRD
))
case let .nonFungible(nonFungible):
state.destination = .nonFungibleTokenDetails(.init(
resourceAddress: resource.resourceAddress,
resourceDetails: .success(resource),
token: nonFungible,
ledgerState: resource.atLedgerState
))
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have this logic when handling the .withdrawals(.delegate(.showAsset(transfer, token))) & .deposits(.delegate(.showAsset(transfer, token))).

We should extract the logic to a shared method so that we perform logic only once and also support missing types (.liquidStakeUnit, .poolUnit & .stakeClaimNFT).

	func resourceDetailsEffect(state: inout State, resource: OnLedgerEntity.Resource, details: ResourceBalance.Details) -> Effect<Action> {
		// Implement switch
		return .none
	}

danvleju-rdx and others added 2 commits July 16, 2024 14:42
…wProofs/TransactionReviewProofs.swift

Co-authored-by: matiasbzurovski <164921079+matiasbzurovski@users.noreply.github.com>
Comment on lines 32 to 33
VStack(spacing: .medium3) {
ResourceBalanceView(proof.resourceBalance.viewState, appearance: .compact) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are adding a VStack for each proof, which is pointless. Need to revert the order, as shown in previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right!

@GhenadieVP GhenadieVP merged commit 3067b2e into main Jul 17, 2024
6 checks passed
@GhenadieVP GhenadieVP deleted the ABW-3569-transaction-review-presenting-badges branch July 17, 2024 15:07
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