-
Notifications
You must be signed in to change notification settings - Fork 5
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
571 - require username in account flow #581
Conversation
@@ -112,27 +112,13 @@ export const ModalScrollContent = styled.div` | |||
` | |||
|
|||
class Modal extends Component<Props, void> { | |||
renderModal () { |
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.
I just moved this method
case MODAL.RESTORE_ACCOUNT: | ||
return <ModalRestoreAccount /> | ||
canClose = () => { | ||
return this.props.modalContent !== MODAL.PROFILE |
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.
You can no longer close the profile modal. The only way to dismiss it is to enter a username (and email if you want) and then continue to the next step.
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.
I think this is very good. My only concern is if the user close or refresh the page - the keystore has been created before that modal, so we have a secure keystore w/ empty user
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.
Yes that is still possible. I think to solve that we would have to rethink the flow a bit and reorder/combine the steps.
For now I think we want to leave the flow as is, plus most users would not do that anyway. But tagging @felipegaucho and @pedrocasa just to get their thoughts.
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 @bent0b0x maybe merge this PR and the we refine the flow
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.
agree. I think that data integrity like this should not be an argument to change the flow here. It is always possible that the user stops halfway (e.g. his battery fails); what we should do is think of policies to recover from that. For example, for this case, we can use a default user name, or, on the next visit, ask the user to provide a username before doing anything else.
onChange={e => this.handleInputChange('email', e)} | ||
margin="0 0 30px" | ||
/> | ||
</form> |
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.
Wrapped this in a form
so that you can submit this modal by pressing enter
Issue
#571
Work Done
Demo