move verification store into modal #3951
Conversation
| onSelectMethod: PropTypes.func.isRequired, | ||
| onClose: PropTypes.func.isRequired | ||
| onClose: PropTypes.func.isRequired, | ||
| isTestnet: PropTypes.bool.isRequired |
There was a problem hiding this comment.
Would prefer consistency. It comes in as isTest, so keep it as isTest (We are doing things in 2 different ways now, some has the one others have the other)
In addition, consider keeping alphabetical order here. (i before o)
| @@ -69,11 +75,12 @@ export default class Verification extends Component { | |||
| } | |||
|
|
|||
| state = { | |||
| method: 'sms' | |||
| method: 'sms', | |||
| store: null | |||
There was a problem hiding this comment.
Why is the store in the state and not on the class itself?
When passed as either a prop or when created on the class it is consistent with what we have elsewhere. Feels really weird having a state in a state. (When done properly, the state should not be on the class using mobx stores at all)
| } else if (name === 'email') { | ||
| store = new EmailVerificationStore(api, account, isTestnet); | ||
| } | ||
| this.setState({ store }); |
There was a problem hiding this comment.
Blank line after the block and before the set would greatly aid readability.
| selectMethod = (choice, i) => { | ||
| this.setState({ method: choice.value }); | ||
| } | ||
| } | ||
|
|
||
| const mapStateToProps = (state) => ({ | ||
| isTestnet: state.nodeStatus.isTest |
There was a problem hiding this comment.
As per above. Have no strong feelings either way, but do have strong feelings about keeping consistency everywhere.
jacogr
added
A8-looksgood 🦄
Fixes #3917. Moves the MobX store into the modal component.