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

Nft Bulk Transfer #134

Closed
wants to merge 21 commits into from
Closed

Nft Bulk Transfer #134

wants to merge 21 commits into from

Conversation

amenconi
Copy link
Contributor

Bulk NFT transfer feature contains 3 main parts:

  1. Checkboxes on eligible NFT cards to allow selection of 1 or more NFTs. Checkbox is available if 2 conditions are met, the logged in user uid matches the nft owner property (you own the NFT) and the nft “locked” property is false. This should allow selection of these NFTs anywhere in Soonaverse that shows the NFT cards (user profile NFTs, Collection NFTs page etc).

  2. NFT selection toolbar. If one or more NFTs are selected a toolbar component opens at the bottom of the screen displaying how many NFTs are selected with a button to clear all selected NFTs and a button to initiate transfer of selected NFTs.

  3. NFT transfer modal. When Transfer button is clicked on the toolbar it opens the transfer modal which lists each selected NFT showing:
    NFT image.
    NFT name.
    NFT network.
    Checkbox to indicate NFT should be transferred.
    Checkbox to indicate transferred NFT should also be withdrawn.
    There is also a text input which will allow input of transfer destination (member or wallet address) and then a Close and Transfer button.
    Field to contain response from Build.5 API once NFT transfer is processed.

Checkbox to withdraw NFT when transferred will be disabled unless the transfer checkbox for that NFT is checked. Transfer button will be disabled until at least one transfer checkbox is checked and a destination is input. Once the first transfer checkbox is checked, all transfer checkboxes for NFTs on an alternate network will be disabled until all transfer checkboxes are unchecked (will remove this if transfers for multiple NFTs on multiple networks can be transferred in a single API call).

You can toggle the recipient selection from wallet address input to selecting Soonaverse member. Selecting Soonaverse member will let you search by Soonaverse account name.

Once transfer selections are made and a recipient name or address is input you can initiate the NFT transfer. Response/error codes returned for each NFT transfer are updated in the transfer modal NFT list. Multiple transfers can be done in the modal iteratively.

When on the profile NFTs page with the NFT transfer modal up, had some issues reactively updating the list of NFTs on profile page in cases where an transfer of one or more NFTs was successful and the transfer modal was closed. For now, in these cases the page will reload to reflect the currently owned NFT for the user. A bit of a hack and leaving the transferred NFTs on the profile NFT page shouldn't open any risks as the API restricts transfers to NFT owned by the logged in user, however it could be confusing. I'll look for better ways to update the list reactively but current PR should have code that works and wanted to open up the feature for core functionality testing.

@amenconi amenconi added the enhancement New feature or request label Feb 28, 2024
@amenconi amenconi self-assigned this Feb 28, 2024
Copy link

cloudflare-workers-and-pages bot commented Feb 28, 2024

Deploying prod-soonaverse-pr-previews with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8056d88
Status: ✅  Deploy successful!
Preview URL: https://fb6287cd.app-cqo.pages.dev
Branch Preview URL: https://amenconi-nft-bulk-transfer.app-cqo.pages.dev

View logs

@adamunchained
Copy link
Contributor

Nice work!

I've just transffered you one of my creamies: https://amenconi-nft-bulk-transfer.app-cqo.pages.dev/nft/0x428a56244a46a1181a56c9e318b26d148d49ac1d

Few notes by quickly playing with it:

  • I think the card should not be clickable once one tickbox is selected
  • "clear" button is invisible to me
  • bottom toolbar could actually have both transfer/withdraw buttons
  • not sure what the status is for and how it's relevant
  • there should be select all option
  • I would select all NFTs upon first opening
  • once I complete transfer it ends up in inconsistent state. Maybe that's were "status" comes in? You intended to update it with success message per NFT?

@amenconi
Copy link
Contributor Author

amenconi commented Mar 1, 2024

Nice work!

I've just transffered you one of my creamies: https://amenconi-nft-bulk-transfer.app-cqo.pages.dev/nft/0x428a56244a46a1181a56c9e318b26d148d49ac1d

Few notes by quickly playing with it:

  • I think the card should not be clickable once one tickbox is selected
  • "clear" button is invisible to me
  • bottom toolbar could actually have both transfer/withdraw buttons
  • not sure what the status is for and how it's relevant
  • there should be select all option
  • I would select all NFTs upon first opening
  • once I complete transfer it ends up in inconsistent state. Maybe that's were "status" comes in? You intended to update it with success message per NFT?

Thanks!

Great point on making card not clickable once selected, definitely a no brainer and will add that ASAP.

Will also fix the font color styling for light theme on the "Clear" button.

For the "Withdraw" button on the toolbar, are you thinking that would bring up a separate modal for withdrawals? I know you can withdraw when transferring but will have to more into how the withdraw mechanism works. I think it just withdraws to the verified address for the user right? So maybe in this case clicking withdraw might not need any further interface and just perform a bulk withdrawal for any selected NFTs?

Status field is populated with the response for each NFT transfer attempt. I've included a video for how it works at least in debug.

Good idea for select all, when opening the transfer modal interface I'll pre-select all for transfer and have a select all toggle for transfer and withdraw checkboxes. I'd assumed that transfers could only happen one a single network at a time but haven't set up purchasing NFTs in debug for other networks yet, though as I think about it, if the transfer is internal then I suppose network shouldn't matter and I can remove the logic I put in to only allow selection of NFTs on a single network at a time.

Also when trying to test on this link: https://amenconi-nft-bulk-transfer.app-cqo.pages.dev/ I'm getting a lot of errors and weird behavior such as it taking a very long time to open selected NFT toolbar or the transfer interface, have you seen the same?

Soonaverse-NFT-Transfer-Test1.mp4

@amenconi
Copy link
Contributor Author

amenconi commented Mar 2, 2024

In latest push I've changed:

  • Fixed light theme selected NFT toolbar buttons
  • Selected NFTs no longer link to NFT page when clicked
  • Opening transfer modal pre-selects all NFTs for transfer
  • Changed header from "Status" to "Result" and pre-populated the field with "Not transferred" on modal open

Currently you can select to withdraw when transferring, would it make sense to add withdraw on the toolbar or add a button per NFT in the transfer modal that allows just withdrawing as a separate action to transfer/withdrawing?

Thanks.

@emmap3-do
Copy link
Contributor

emmap3-do commented Mar 8, 2024

@amenconi the footer is not visible when I'm on my Profile NFTs page. It is visible if I'm on a collection page and select my NFT there.

I would expect, as a final user, to see the footer on Profile > NFTs page.

Also, should selected NFTs be stored in local storage? Because if I refresh the page, footer is gone and selected NFT is no longer selected.

Same thing for multiple browser tabs opened.

@amenconi
Copy link
Contributor Author

amenconi commented Mar 9, 2024

@amenconi the footer is not visible when I'm on my Profile NFTs page. It is visible if I'm on a collection page and select my NFT there.

I would expect, as a final user, to see the footer on Profile > NFTs page.

Also, should selected NFTs be stored in local storage? Because if I refresh the page, footer is gone and selected NFT is no longer selected.

Same thing for multiple browser tabs opened.

Footer should definitely show no matter the page you are on, messaged you on Discord to discuss more in-depth.

My initial design was to only track selected NFTs in memory rather than persist it with local storage. My thoughts were that selecting NFTs and then doing something with that selection (such as transferring) would likely be an immediate process rather than something that would make sense to span browser sessions as opposed to the shopping cart where you might put a few items in the cart and then want to come back later and pick up your shopping process before pulling the trigger on a purchase. I may be thinking of this wrong or missing something though and definitely open to saving selected NFTs in local storage if you think that would be preferable.

Thanks.

@emmap3-do
Copy link
Contributor

I think you have a point regarding that the selection of NFTs for a bulk transfer would be a immediate process, so I think it makes sense not to span the process across multiple browser tabs. I'm not sure though about a process where someone is setting up the bulk process and accidentally reloads the page or goes elsewhere and then comes back to the process and find it was reset. Though it's not a big deal (only if the user had set up like hundreds of nfts). Either way, I think this can be left handled as is, let the users use it and give feedback, then adjust to a more complex feature if needed.

@amenconi
Copy link
Contributor Author

This latest push includes:

  • In testing some users were having errors in listing their recent spaces in the Members About section loaded with the Member Page no matter which sub page was loaded (activity, spaces, nfts etc.). For some reason this seemed to be causing issues with the NFT transfers when attempted from the member's NFTs page. This should include a fix where member and space array objects populated in the Data Service are sanitized to only include items that are valid and have a populated "UID" property.
  • Modifications were made to help make it more clear on what occurred for the NFT transfers:
  1. NFT transfer modal NFT rows change to Red/Green backgrounds to indicate if NFT succeeded or failed to be transferred during session (resets when NFT transfer modal is closed and re-opened for any reason).
  2. nzNotifications added to send notification indicating number of successful and failed NFT transfers.
  3. After NFTs transfer is processed a closable modal is popped up indicating how many NFTs succeeded in transfer and how many failed and alerts user to look at NFT transfer modal "Results" field for detailed error message to indicate issue in cases where the transfer failed.

Hopefully this makes it more clear to user what happened so they can make changes and attempt transfer again if appropriate.

I'm also looking into sending a message to user that transferred and user that received NFT during each transfer. On the back end these are named "Notification" but are different than the nzNotifications from the ng-zorro-antd library and show up when the Bell icon is clicked on the header. Initially I see that the Notification being referenced from Build.5 interface library is deprecated so it may take a bit more research before implementing this. However I feel this may be a more nice to have than anything critical and I think we can release this feature without it if it's not added in a timely manner.

Thank you.

@emmap3-do
Copy link
Contributor

Thank you @amenconi for these last improvements!

A few things I found today:

  • When using Firefox, I cannot click the checkbox to select the NFT to be transferred (checkbox hover style works, but clicking on it redirects me to the NFT details page instead of adding it to the footer).
  • Animated NFTs fail to show the image in the modal popup window.
  • For some NFTs it fails to show the network.
  • May be redundant, but I think it's good to be consistent throughout different features and modules: could you please add the "Clear" button to the modal popup footer? (same as you did for the NFT cart feature).
  • Next to the "Transfer" and "Withdraw" column titles may be nice to add the question mark icon (same as we have in the checkout modal popup, next to the royalties percentage). When the user hovers over the icon, display a text explaining the difference or what to expect.
    Eg. something like this:
    Transfer: Selected NFTs will be transferred and automatically deposited into the recipient's Soonaverse Profile.
    Withdraw: Selected NFTs will be transferred and withdrawn into the recipient's verified wallet address.

image

Thank you!

@amenconi
Copy link
Contributor Author

Great feedback as always, thank you.

  • Will do some testing on Firefox as well, not sure why the behavior might be different but will coordinate with you if I can't reproduce for any reason.
  • I'll have to look into animated NFT images, thinking there might be some issue with them in calling preview and will see how it's handled elsewhere.
  • I think in other places where it can't get the network it falls back to "DEFAULT_NETWORK", will make sure this applies the same logic.
  • Yes makes sense, will add Clear button. Since there won't be any NFTs selected at this point I'll just have it close the modal as well, then the user can reselect NFT from scratch.
  • Tooltips are a great idea, anywhere else you can think of where it might help to add them?

Thank you.

@amenconi
Copy link
Contributor Author

Ok this push should fix the following:

  • Detects if nft media is video and displays video media in that case. Also if image media is no populated will show a default image. I did see on debug some NFTs were images and the media property was populated but the url went to no image so the image icon in transfer modal did look like a broken image, not sure if there is much we can do about that.
  • If NFT has no network listed it will default to "DEFAULT_NETWORK" as defined in Build.5 API.
  • Clear button added
  • Added info icons with tooltips for "Transfer" and "Withdraw" column headers.

Will continue to do testing on Firefox and let you know once I can reproduce the issues and hopefully solve.

Thank you.

@amenconi
Copy link
Contributor Author

Pulled in latest develop branch state and resolved merge conflicts.

I also believe I've fixed the Firefox issue. Oddly persistent problem that took a few iterations to solve but this version was working well on Chrome and Firefox in debug at least.

Also did some light UI styling changes to better match the buttons on shopping cart modal, nft selection toolbar and nft transfer modal.

Thank you.

@emmap3-do
Copy link
Contributor

I confirm the bug is fixed on Firefox. Everything seems to be working properly. And UI looks better now with those adjustments.

I only have a few comments that would smooth user experience, but not really a blocker for this feature to go live:

  • Maybe it would be best to have the "specific member" selection set by default, instead of the wallet address.

  • When the transfer succeeded and modal is closed, the page does a full reload. If it simply removes the NFTs from view, instead of reloading the page, would be a better user experience.

  • If the recipient user had their profile NFT tab opened, and NFTs arrive, they won't notice until a manual page refresh is made or they navigate to another page and navigate back to their NFT profile tab.

@emmap3-do
Copy link
Contributor

@amenconi maybe we can move this feature live, given that it's functional and my comments were only ux improvements.

What do you guys think @adamunchained @SoonaverseTF ?

@emmap3-do
Copy link
Contributor

It seems that this version of the app is no longer working, when I try to connect to the staging URL I get the error:
"Failed to initialize wallet, try to reload page.".

@adamunchained
Copy link
Contributor

It seems that this version of the app is no longer working, when I try to connect to the staging URL I get the error: "Failed to initialize wallet, try to reload page.".

Test account has been shutdown as it has nearly no use. Did you try to point to prod environment instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants