Seed mobile #785
Seed mobile #785
Conversation
|
Probably best to rebase this on top of the master once #782 is merged. |
valentinewallace
force-pushed
the
seed-mobile
branch
3 times, most recently
from
5141556 to
0a5af61
Compare
valentinewallace
force-pushed
the
seed-mobile
branch
2 times, most recently
from
a45acd5 to
657e506
Compare
valentinewallace
force-pushed
the
seed-mobile
branch
3 times, most recently
from
2813ad8 to
028e896
Compare
There was a problem hiding this comment.
@valentinewallace I gave it a first rough pass. Also, please rebase on the current master.
src/action/auth-mobile.js
Outdated
| this._nav.goSelectSeed(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This stuff should be in the wallet actions. Auth-mobile is only for PIN and TouchID logic.
src/view/seed-mobile.js
Outdated
| style={styles.title} | ||
| keepCase | ||
| title="Write down each word and store it in a safe place." | ||
| > |
There was a problem hiding this comment.
Title isn't semantically correct here. We should use one of the Text of Copy components.
src/component/header.js
Outdated
| </Text> | ||
| </View> | ||
| ); | ||
|
|
||
| Title.propTypes = { | ||
| title: PropTypes.string, | ||
| style: View.propTypes.style, | ||
| keepCase: PropTypes.bool, |
There was a problem hiding this comment.
Changing Title shouldn't be necessary if we use a Text or Copy component.
valentinewallace
force-pushed
the
seed-mobile
branch
2 times, most recently
from
98d1a6f to
3c049eb
Compare
There was a problem hiding this comment.
@valentinewallace I refactored the seed-mobile view, and header components to be a bit more usable. Since we'll probably reuse them in other screens. Let me know what you think.
Gonna review the rest of the PR tomorrow.
There was a problem hiding this comment.
@valentinewallace I finished reviewing the rest of the PR. Looks really good now 👍 Good to merge from my side if you agree with my changes.
|
@tanx cool, checking it out! |
valentinewallace
force-pushed
the
seed-mobile
branch
from
7f64f3d to
f5080b7
Compare
|
@tanx I removed an unnecessary word in the unit tests. Good to merge when you are =] |
Closes #581 and #579.
Going to rebase on top of #783 (Auth mobile) when it's merged.