Skip to content
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

Allow to set recovery phone when creating account #29

Merged
merged 22 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@vgrichina
Copy link
Member

commented Apr 27, 2019

No description provided.

@vgrichina vgrichina force-pushed the set-recovery branch from 32627c1 to ac388fc May 2, 2019

@vgrichina vgrichina force-pushed the set-recovery branch from bdd137b to ee23ed5 May 10, 2019

@vgrichina vgrichina changed the title WIP: Allow to set recovery phone Allow to set recovery phone when creating account May 11, 2019

@vgrichina vgrichina requested a review from janedegtiareva May 11, 2019

@@ -41,6 +43,17 @@ export function handleRefreshAccount(wallet, history) {
})
.catch(e => {
console.log(e)

if (e.message && e.message.indexOf('is not valid') !== -1) {

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

pull 'is not valid' into a constant somewhere?

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

or better yet this function might be good as a util function

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 13, 2019

Author Member

proper way to fix this would be to haver error codes returned by nearcore. This works as a temporary kludge as is.

}))
}

isLegitField(name, value) {

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

let's rename legit to something else, maybe valid?

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 14, 2019

Author Member

this seems subjective, Legit is used in all other code in wallet.

</Header>
<Header as='h3' className='color-blue'>
You can skip this if you plan to backup account keys manually.
We won't be able to help you with account recovery otherwise.

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

Is this text from the official UX? I think the sentence should read as "You can skip this blah blah... If you do skip this, we won't be able to help with your account recovery" or something similar. Right now it reads as having the opposite meaning of what we want to say.

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 13, 2019

Author Member

There is no official UX for now, going to update text after comments from Jake and Erik

<Grid.Row className='disclaimer border-top-bold'>
<Grid.Column computer={16} tablet={16} mobile={16} className=''>
<span className='disclaimer-info'>DISCLAIMER: </span>
This is a developers&apos; preview Wallet. It should be used for

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

Is this still true about the preview?

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 13, 2019

Author Member

yes

import styled from 'styled-components'

const RecoveryInfoForm = styled(Form)`
&&& input {

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

is it normal in react to have css inline? I definitely prefer separate css files

This comment has been minimized.

Copy link
@potatodepaulo

potatodepaulo May 13, 2019

Member

is it normal in react to have css inline? I definitely prefer separate css files

This is somewhat common in react for component-level styles that aren't going to be reused elsewhere to avoid css anti-pattern of overriding, then overriding again with !important. I'm not personally a fan, but this is pretty widely accepted.

This comment has been minimized.

Copy link
@potatodepaulo

potatodepaulo May 13, 2019

Member

That being said, this is a big enough block of style that I would argue it should be taken out of here.

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 13, 2019

Author Member

that's normal pattern, the way to refactor it is to cut components into smaller pieces

the good part about this is that it's scoped by component, so very easy to manage vs global styles which can get applied to whatever

return false
}

const { dispatch } = this.props;

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva May 13, 2019

Member

Let's start adding tests for the wallet code. This looks like it doesn't have a lot of UI logic, so should be possible to write unit tests for this.

This comment has been minimized.

Copy link
@vgrichina

vgrichina May 13, 2019

Author Member

I plan to add integration tests for contract helper and end-to-end tests for wallet.

Unit tests are going to be easy to write for Redux-based app, but I don't think they are going to be very useful. Most of our real world problems are with broken integrations.

@janedegtiareva
Copy link
Member

left a comment

Requested changes:

  • review the text
  • some small nits (can be addressed in follow up if needed)
  • let's add some unit tests

@vgrichina vgrichina referenced this pull request May 14, 2019

Merged

Recover account #35

@vgrichina vgrichina merged commit d6b10ae into react May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.