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

DAO Creation Token configuration fixes #2248

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

mudrila
Copy link
Contributor

@mudrila mudrila commented Aug 14, 2024

  • Hide token must be wrapped message until we actually resolve whether imported token is votes token or not.
  • Disable token name and symbol inputs if imported token is votes token

Closes #2236
Closes #2237

… imported token is votes token or not. Disable token name and symbol inputs if imported token is votes token
@mudrila mudrila added bug Something isn't working enhancement New feature or request maintenance Keep the lights on labels Aug 14, 2024
@mudrila mudrila self-assigned this Aug 14, 2024
Copy link

netlify bot commented Aug 14, 2024

Deploy Preview for decent-interface-dev ready!

Name Link
🔨 Latest commit daf1ee3
🔍 Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/66bcf3a2af1171000859aac2
😎 Deploy Preview https://deploy-preview-2248.app.dev.decentdao.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -177,7 +179,7 @@ export function AzoriusTokenDetails(props: ICreationStepProps) {
isRequired
/>
</LabelWrapper>
{!isImportedVotesToken && !errors.erc20Token?.tokenImportAddress && (
{isImportedVotesToken === false && !errors.erc20Token?.tokenImportAddress && (
Copy link
Member

Choose a reason for hiding this comment

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

Yes nice

@nicolaus-sherrill
Copy link

Three things I spotted:

  1. There is a bit of a delay between inputting the token address and the "wrapped" message popping up (alongside token info)
  2. I'm still able to proceed to the next step to deploy the Safe, despite this message — should this be possible?
  3. Bug happening when clicking on the space around the token address input field that changes my selection to "New Token"

recording

Browser metadata
Path:      /create/erc20Token
Browser:   Chrome 127.0.0.0 on Mac OS 10.15.7
Viewport:  1465 x 904 @2x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@nicolaus-sherrill
Copy link

Three things I spotted:

  1. There is a bit of a delay between inputting the token address and the "wrapped" message popping up (alongside token info)
  2. I'm still able to proceed to the next step to deploy the Safe, despite this message — should this be possible?
  3. Bug happening when clicking on the space around the token address input field that changes my selection to "New Token"

recording

Browser metadata
Open Deploy Preview · Mark as Resolved

I guess I see now after watching my recording back that we are wrapping the token. That is not clear at all that that is what is happening while in-the-flow when moving/clicking/deciding at normal behavior speeds. Big UX flag for me there.

@nicolaus-sherrill
Copy link

When inputting an address for a wrapped token, I'm given that same Wrapped message and the name now says "wrapped wrapped"

screenshot

Browser metadata
Path:      /create/erc20Token
Browser:   Chrome 127.0.0.0 on Mac OS 10.15.7
Viewport:  1465 x 904 @2x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

Looks like there is a GIthub Lint warning, but approved otherwise

@adamgall
Copy link
Member

When inputting an address for a wrapped token, I'm given that same Wrapped message and the name now says "wrapped wrapped"

Oh that's funny, I would expect that a Wrapped token implements IVotes already (which I believe is the thing that matters when determining whether or not the given token needs to be wrapped or not), so I wonder why we'd attempt to rewrap it.

@mudrila would you mind looking into that.

@mudrila
Copy link
Contributor Author

mudrila commented Aug 15, 2024

When inputting an address for a wrapped token, I'm given that same Wrapped message and the name now says "wrapped wrapped"

Oh that's funny, I would expect that a Wrapped token implements IVotes already (which I believe is the thing that matters when determining whether or not the given token needs to be wrapped or not), so I wonder why we'd attempt to rewrap it.

@mudrila would you mind looking into that.

@adamgall
Yup, I'll look into that
Though I think we should merge this PR and tackle that in scope of another issue, as well as other bug that @nicolaus-sherrill discovered. WDYT?

@adamgall
Copy link
Member

The Github Lint warning on this PR doesn't make any sense lol

@adamgall
Copy link
Member

@mudrila new issues for those items sounds good to me. Please get those created then feel free to merge this.

Thanks for your review @nicolaus-sherrill!

@adamgall
Copy link
Member

@nicolaus-sherrill regarding

  1. There is a bit of a delay between inputting the token address and the "wrapped" message popping up (alongside token info)
  2. I'm still able to proceed to the next step to deploy the Safe, despite this message — should this be possible?

Yes all of this is expected.

  1. Delay is because there's a network call happening there
  2. The info message is not a warning or error (blocking), just informational (non blocking)

Also understood, that you call into question the UX in general.

@mudrila
Copy link
Contributor Author

mudrila commented Aug 15, 2024

@adamgall @nicolaus-sherrill Created couple of issues - one for further development and one for design

#2253
#2252

So gonna merge this now

@mudrila mudrila merged commit 120a3fa into develop Aug 15, 2024
7 checks passed
@mudrila mudrila deleted the fix/azorius-creation-token-inputs branch August 15, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request maintenance Keep the lights on
Projects
None yet
5 participants