-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
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.
Good job.
Some details to be fixed below.
- the last step has button "Login", should say "Register" instead.
- once in the "Enter missing word" step it showed all 12 words and "---" after them, the button was enabled.
!props.account.secondSignature ? | ||
<MenuItem caption="Register second passphrase" | ||
onClick={() => props.setActiveDialog({ | ||
title: 'Second Passphrase', |
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.
IMO should be consistent with caption="Register second passphrase"
onPassGenerated: onLoginSubmission, | ||
keepModal: true, | ||
}, | ||
})}/> : <li className={`empty-template ${styles.hidden}`}></li> |
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 not just })}/> : null
?
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.
because a component can not return null. it's possible when you're doing this in the middle of a template when at the end, the component won't return null, but not here.
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.
It can return null
https://stackoverflow.com/questions/42083181/is-it-possible-to-return-empty-in-react-render-function
I just tested it.
@@ -0,0 +1,3 @@ | |||
.hidden { | |||
display: none; | |||
} |
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 file can be deleted after the resolving comment on line that uses it.
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.
since as I mentioned above, I'll have to use it, then this css rule also needs to stay
}; | ||
|
||
it('renders one MenuItem component for a normal account', () => { | ||
const props = { |
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.
To prevent code duplication, props
can be defined outside it
s without account
. Appropriate account
can be set inside each it
.
yes, I've also seen the issue with missing word. there are other parts I want to tweak in passphrase component. I'll create a ticket fix them up all together. |
Now that pending transactions PR is merged this should also dispatch a pending transaction (see how it's used in send send https://github.com/LiskHQ/lisk-nano/blob/development/src/components/send/send.js#L74-L81 ) |
…k-nano into 353-second-passphrase
91ccb07
to
bfca491
Compare
I've implemented the requested changes and Vit is not here to approve
Closes #353