Skip to content

1003 update managetoken component#1005

Merged
ericsharma merged 6 commits into942-feature-algodex-launchpad-for-creating-and-listing-asasfrom
1003-update-managetoken-component
Mar 16, 2023
Merged

1003 update managetoken component#1005
ericsharma merged 6 commits into942-feature-algodex-launchpad-for-creating-and-listing-asasfrom
1003-update-managetoken-component

Conversation

@treasurechic
Copy link
Contributor

ℹ Overview

closes #1003

📝 Related Issues

🔐 Acceptance:

  • yarn test passes

@treasurechic treasurechic linked an issue Mar 10, 2023 that may be closed by this pull request
@vercel
Copy link

vercel bot commented Mar 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
algodex-react-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 15, 2023 at 8:47PM (UTC)

@treasurechic treasurechic changed the base branch from development to 942-feature-algodex-launchpad-for-creating-and-listing-asas March 10, 2023 11:58
Copy link
Collaborator

@ericsharma ericsharma left a comment

Choose a reason for hiding this comment

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

Only the active editable field should be in focus. If I click edit on a second field then the first field should automatically close. See below screen shot for incorrect behavior. Only the field that the user most recently clicked should be in the editing state.

Screen Shot 2023-03-10 at 8 34 06 AM

@treasurechic
Copy link
Contributor Author

treasurechic commented Mar 10, 2023

@ericsharma I don't think this behavior is really cool for the users tho. So what if a user is editing a field but has not yet clicked "cancel" or "confirm" on that field but proceeds to click the "edit" icon of another field, with the intention of clicking the "confirm" button on each field after pasting all the addresses in.

So according to your request now, what do you think I should do for this user?.... Cancel the current input? (which will revert to the old address) or confirm the current input(which will save the address on the input UI).

IMO, I think it is ok for users to be able to edit multiple fields at the same time.

@ericsharma
Copy link
Collaborator

@treasurechic There is no reason for a user to have multiple editable fields open at the same time, what would be the purpose? There is no button to submit edits in bulk so why have the option to have multiple editable fields if you have to explicitly confirm each individual one? In my opinion it is bad UI and just opens up more surface area for unexpected behavior. I'm not sure why a user would ever want to do this.
And to answer your question: If a user does not explicitly hit confirm then you revert the field to what was there before.

@stephclarkga Perhaps you can weigh in? I'd defer to your judgement since this is your area of expertise.

@treasurechic treasurechic requested review from stephclarkga and removed request for stephclarkga March 14, 2023 10:04
@ericsharma
Copy link
Collaborator

@stephclarkga Any update on this? PR is waiting for your feedback.

@stephclarkga
Copy link

stephclarkga commented Mar 14, 2023

I agree that only one field should be in the editing state at a time and if an entry is not confirmed when another is open, it should revert to the previous address. I also like the idea of only having one editable field at a time since it helps ensure the users actually hit the check to submit the change instead of editing and just hitting the final submit button overall

I think we need something to show that the address was changed (before they click the main button to send to blockchain) to give confirmation and call out that it was updated. Maybe if an address is edited, it could be shown in green until submitted? I also wonder if we need to add a call out once an address is edited to remind the user that they need to submit the changes to the blockchain to finalize it.

@treasurechic
Copy link
Contributor Author

treasurechic commented Mar 14, 2023

@stephclarkga pls can you update the ui with the illustration of the second paragraph of your comment. So I can update the interface accordingly.

@stephclarkga
Copy link

stephclarkga commented Mar 14, 2023

@treasurechic I updated the Figma with this:
Screen Shot 2023-03-14 at 10 01 15 AM

If any field is edited, when the edit field is closed, this info message shows up below it. I changed my mind about the addresses being a different color though- I think it reads fine in white with the call out message below it. I also added some text to the general callout above the "Update Token" button.

@stephclarkga
Copy link

stephclarkga commented Mar 15, 2023

@treasurechic Trying to view the "Manage Token" page but I'm getting this error when I have a wallet connected and try to view that page: Screen Shot 2023-03-15 at 11 54 51 AM
Screen Shot 2023-03-15 at 11 54 32 AM

Without a wallet connected, I can view the page but there's not assets to select from

@treasurechic
Copy link
Contributor Author

treasurechic commented Mar 15, 2023

@stephclarkga this is because the wallet you connected with has no "created asset" it can manage.

A fix has been deployed for this error. Pls test again.

However, If you haven’t created an asset with the connected wallet, you will have no asset to select from as you can’t manage an asset you did not own or assigned as its manager.

@stephclarkga
Copy link

Ah, I figured that might be the case. I can view the page with the wallet connected now so that's good. I did try to create a token but wasn't sure if that was working. Just tried again and got this weird error:
Screen Shot 2023-03-15 at 1 16 36 PM
Screen Shot 2023-03-15 at 1 18 50 PM

@treasurechic
Copy link
Contributor Author

@stephclarkga I will test and get back to you on this create token error.

For you to proceed with your test on the manage token page, you can create a token with your wallet on another platform so you can have assets to test with.

Copy link

@stephclarkga stephclarkga left a comment

Choose a reason for hiding this comment

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

Editing the fields works well 👍🏻 I wasn't able to submit the update (button didn't do anything?) but I assume that hasn't been done yet?

Couple small design things:

  1. Can you add a little space between the wallet address and the copy button? (probably 4px). Just makes it a little more readable. Should be similar spacing to the creator wallet address at the top in the box.

Screen Shot 2023-03-15 at 2 13 26 PM

  1. I think the "total supply" is display with a comma instead of a decimal. This asset I created has a total of 16 but it shows as 16,000,000. I think all created assets have to be a whole number so I think it's best for it to show the total supply without any decimals (since we have the decimal number in the next line) but let me know if that's not possible or why it's showing like this.

Screen Shot 2023-03-15 at 2 14 50 PM

@treasurechic
Copy link
Contributor Author

@stephclarkga yes, the button is not connected to the actual update yet.

The total supply was a wrong calculation on my part which I have fixed and updated.
Yes, the total supplies are whole numbers no decimal is allowed.

Copy link

@stephclarkga stephclarkga left a comment

Choose a reason for hiding this comment

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

Looks good now! 👍🏻

@ericsharma ericsharma merged commit e6e2bbf into 942-feature-algodex-launchpad-for-creating-and-listing-asas Mar 16, 2023
@ericsharma ericsharma deleted the 1003-update-managetoken-component branch March 16, 2023 13:37
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.

Update ManageToken component

3 participants