Generate Mnemonic Wallet #659
Conversation
…stuff. Remove generate help section. Fix styles.
wbobeirne
added
the
status: ready for review
wbobeirne
requested review from
HenryNguyen5,
dternyak,
skubakdj and
james-prado
|
Tested, works great! Implementation looks great as well. I did notice that there is no way to reset or go back once you have selected a keystore / mnemonic based wallet generation. It would be nice to have a restart / back button, but it doesn't need to hold up the merge. |
| walletType: WalletType; | ||
| } | ||
|
|
||
| export default class FinalSteps extends React.Component<Props> { |
There was a problem hiding this comment.
This could just be an SFC instead, and then you can have all the logic in a separate function too
| </p> | ||
|
|
||
| <div className="GenerateMnemonic-words"> | ||
| {[firstHalf, lastHalf].map((ws, i) => ( |
There was a problem hiding this comment.
I would consider unrolling the first level array just to reduce complexity, with having a helper function / SFC generate the Word components
There was a problem hiding this comment.
Can you elaborate on
unrolling the first level array
Not sure what that looks like
There was a problem hiding this comment.
I think I simplified this by making a makeWord helper function.
| }; | ||
|
|
||
| private handleConfirmChange = (index: number, value: string) => { | ||
| console.log(index, value); |
| const confirmValues = [...this.state.confirmValues]; | ||
| confirmValues[index] = value; | ||
| console.log(confirmValues); | ||
| this.setState({ confirmValues }); |
There was a problem hiding this comment.
This could all be done with a setState cb that'll give you the previous state. Should be more clean.
| <div className="MnemonicWord-word input-group"> | ||
| <input | ||
| className={classnames({ | ||
| 'MnemonicWord-word-input': true, |
There was a problem hiding this comment.
This can be shortened to
classnames(
'MnemonicWord-word-input',
'form-control',
{'is-valid': word === value}
)There was a problem hiding this comment.
Or using the same style as LN 49 to keep it consistent
| </div> | ||
| </div> | ||
| ); | ||
| return <div className="GenerateWallet Tab-content-pane">{this.props.children}</div>; |
| onSelect(type: WalletType): void; | ||
| } | ||
|
|
||
| export default class WalletTypes extends React.Component<Props, {}> { |
There was a problem hiding this comment.
Would be dope if there was a linter rule or something that caught implementations of React.Component that only had a render method. I'll convert this, but lmk if you think that's out there.
|
Personally I find keeping the markup as together as possible makes reading the structure of the markup a little easier, but I can pretty easily convert some of those maps to helper functions. |
|
@dternyak added a UI back button, but also refactored this so it no longer uses state to keep track of which wallet we're generating, but uses the router instead. This makes the back button work as well. Added routes for /generate, /generate/keystore, and /generate/mnemonic.
@HenryNguyen5 I think I addressed everything! Let me know if I got any changes wrong. |
|
I alertified the warning text upfront, and made the other wallets actual links:
|
Closes #109, and addresses a lot of #557. This PR looks a lot bigger than it really is, so don't let the line count fool ya. This reworks the generate flow to give users the option between a keystore file and a mnemonic phrase. You can see #109 for the discussion around all of the changes, or you can just watch the following example videos:
Gfycat - Mnemonic wallet generation
Gfycat - Updated keystore wallet generation
There were a few additional changes related to this: