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-1666 bBlock duplicate NFT transfer #551

Merged
merged 5 commits into from
Jun 7, 2023

Conversation

kugel3
Copy link
Contributor

@kugel3 kugel3 commented Jun 6, 2023

Jira ticket: ABW-1666

Description

  • Blocks the ability to try to send the same NFT to multiple receiving accounts
  • Fixes the appearance of collapsed NFT lists

Screenshot

The upper NFT has already been selected for another account
image

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

@kugel3 kugel3 requested a review from GhenadieVP June 6, 2023 22:05
Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

LGTM, though I think we might have wanted to still allow users to select the NFT but display an error in AssetTransfer View. Android I think does exactly that.

@kugel3
Copy link
Contributor Author

kugel3 commented Jun 7, 2023

We could allow them to tap it and show the error message then. That's why I put the selection blocking logic in the reducer instead of the view, so we can choose what to do. I definitely think we should indicate visually that it's not selectable.

@GhenadieVP
Copy link
Contributor

We could allow them to tap it and show the error message then. That's why I put the selection blocking logic in the reducer instead of the view, so we can choose what to do. I definitely think we should indicate visually that it's not selectable.

I think idea is to follow the same UX as for fungible resources having the total sum going over the balance, in a way sending the same nft to two different accounts is going over the balance.
We could re-iterate this with Matt, but the last consensus was to show the error in the AssetTransfer.

@kugel3
Copy link
Contributor Author

kugel3 commented Jun 7, 2023

That's a different UX though. For fungibles, you get told that you're over the limit right away, when you enter the amount. So you get warned right where you can change it. It's not very nice if you have to go back in to a receiving account and change it. Also, it's hard to convey which NFT or NFTs that are duplicated, they might forget before they get in to the asset screen.

I guess we could remove the indication that it has been disabled, and only show an error after they select it, but right there on the asset screen.

@GhenadieVP
Copy link
Contributor

That's a different UX though. For fungibles, you get told that you're over the limit right away, when you enter the amount. So you get warned right where you can change it. It's not very nice if you have to go back in to a receiving account and change it. Also, it's hard to convey which NFT or NFTs that are duplicated, they might forget before they get in to the asset screen.

I guess we could remove the indication that it has been disabled, and only show an error after they select it, but right there on the asset screen.

I am not rooting for any UX, both seem reasonable for me.
I am ok with merging this, and polish later when we do actually get UX input, as we made this kind of decision Ad-Hoc.

@kugel3 kugel3 merged commit 1ad2cc6 into main Jun 7, 2023
@kugel3 kugel3 deleted the ABW-1666_Block-duplicate-NFT-transfer branch June 7, 2023 08:26
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.

2 participants