Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Set second passphrase - Closes #353 #552

Merged
merged 21 commits into from
Aug 10, 2017
Merged

Conversation

reyraa
Copy link
Contributor

@reyraa reyraa commented Aug 3, 2017

  • Uses PricedButton in ActionBar, therefore it needs to adapt the unit tests in many components.
  • Creates a button triggering a modal to generate and register a second passphrase to current account.
  • Also includes some minor eslint fixings on other components.

Closes #353

slaweet
slaweet previously requested changes Aug 4, 2017
Copy link
Contributor

@slaweet slaweet left a 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',
Copy link
Contributor

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just })}/> : null?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@
.hidden {
display: none;
}
Copy link
Contributor

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.

Copy link
Contributor Author

@reyraa reyraa Aug 4, 2017

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 = {
Copy link
Contributor

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 its without account. Appropriate account can be set inside each it.

@reyraa
Copy link
Contributor Author

reyraa commented Aug 4, 2017

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.

@slaweet
Copy link
Contributor

slaweet commented Aug 4, 2017

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 )

@reyraa reyraa requested review from alepop and removed request for yasharAyari August 9, 2017 10:31
@reyraa reyraa force-pushed the 353-second-passphrase branch from 91ccb07 to bfca491 Compare August 10, 2017 10:52
@reyraa reyraa requested a review from karmacoma August 10, 2017 15:52
yasharAyari
yasharAyari previously approved these changes Aug 10, 2017
@reyraa reyraa dismissed slaweet’s stale review August 10, 2017 15:58

I've implemented the requested changes and Vit is not here to approve

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Register second passphrase to React
4 participants