-
Notifications
You must be signed in to change notification settings - Fork 20
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
Easy flow to set an identity #404
Conversation
This is a really nice improvement, working great for adding and updating, all working as expected. I found a crash in the case where an address is pasted into one of the inputs (see the video where I just copied from the identicon and then pasted). Pasting in normal strings works fine. CleanShot.2023-10-16.at.15.36.18.mp4 |
Ah great catch, I didn't realize there was a limit of char. I'll implement this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UI works as expected, I was able to create a transaction and sign and my multisig name was changed.
TBH for me as a quite new person to the Polkadot ecosystem the field data labels are confusing, it's hard to guess where all this data would be reflected. Can we at least add input placeholders to give more information about the data we ask?
export const easyTransferTitle = [ | ||
'Send tokens', | ||
'From call data', | ||
'Manual extrinsic', | ||
'Set identity' | ||
] as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it's a case to enum:
enum EasyTransferTitle {
SendTokens = 'Send tokens',
FromCallData = 'From call data',
ManualExtrinsic = 'Manual extrinsic',
SetIdentity = 'Set identity',
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I believe you misinterpret the strict typing that is used here. This is used as a type (right next line), and allows to strongly type anything that uses EasyTransferTitle
while remaining very easy to read. Also it allows to do things like easyTransferTitle.included(val)
to check whether this value passed is actually of the type we expect, and not just a random string.
return { | ||
[SEND_TOKEN_MENU]: ( | ||
'Send tokens': ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With enum, it will be easier to use the value without specifying a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no a string, this is a EasyTransferTitle
. You can't put any string in there without a type error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw the type, it's still confusing to read. I could understand if you created a type with one-word strings: like active | disabled | focus and used them this way. But for a 2-3 word string, it's probably the case for an enum where you don't use directly the value but the link to it. You can use the whole sentence as a type, but it doesn't mean you should do it. It works both ways, but in my opinion with an enum, it gives better readability and expectation that this is a predefined value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, not willing to fight too much for this one, I agree that enum feels safer (although it's actually the same). Did the change.
Sure that's a good idea. Just to be clear, Multix, and multisigs in general are not for ppl new to Polkadot, but it doesn't mean that I can use this as an argument for everything! The goal of this component is to make it so that ppl don't have to use polkadot.js/apps, they had no other option this far. Pjs has an easy flow for setting an identity that looks similar. |
… tbaut-set-identity
I hope you had the chance to test it out, because on Rococo it's now filtered 😭 because of paritytech/polkadot-sdk#1476 |
The recent updates, character limits, placeholder text, and title updates, are nice additions. The "Display name" error and highlighted field appear immediately upon loading the modal, which effectively signals that it's a mandatory field. This seems fine to me. |
Yeah that's not good, I'll change this before merging. |
closes #373
Adds an easy setup to set or modify and on-chain identity
Submission checklist:
Layout
Compatibility