Skip to content
This repository was archived by the owner on Feb 23, 2021. It is now read-only.

Conversation

@tanx
Copy link
Contributor

@tanx tanx commented Jan 14, 2019

Closes #582

@tanx tanx requested a review from valentinewallace January 14, 2019 03:28
@tanx tanx mentioned this pull request Jan 14, 2019
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Very happy to see mobile coming together! The fact that this is about to be merged shows how much progress has been made in just a few months.

I found the commit history a bit difficult to follow (e.g. this commit 23319fb appears to re-add the SeedVerify view but the commit message just mentions a navigation change). Personally i'd prefer if the commits were rebased and cleaned up a bit/made more descriptive before merging.

However, the final code looks good and works well 👌 So let me know what you think! Feels good to conquer those tricky TextInputs.

@tanx
Copy link
Contributor Author

tanx commented Jan 15, 2019

Very happy to see mobile coming together! The fact that this is about to be merged shows how much progress has been made in just a few months.

Yeah, the final screen for the mobile MVP is about to be merged. So exciting 🎉

I found the commit history a bit difficult to follow (e.g. this commit 23319fb appears to re-add the SeedVerify view but the commit message just mentions a navigation change). Personally i'd prefer if the commits were rebased and cleaned up a bit/made more descriptive before merging.

Sorry about that. My initial plan was to untangle the original PR into two (one for the seed verify view and one for text input focusing). So I removed the textinput changes first, cleaned up the seed verify view, and then planned to add text-input focus on a second rebased branch. It turned out that my solution came very close to your initial PR. So I thought it would be easier for you to review the final result. I can split them into two PRs if it's easier for you.

However, the final code looks good and works well 👌 So let me know what you think! Feels good to conquer those tricky TextInputs.

Definitely. There are several npm packages that do nothing except textinput focus and form handling in react native. Doesn't seem to be a trivial thing. I'm glad we found a simple solution that works 👍

@tanx tanx force-pushed the seed-verify-mobile-rebased branch from 23319fb to 0c92bd1 Compare January 15, 2019 04:29
@tanx
Copy link
Contributor Author

tanx commented Jan 15, 2019

@valentinewallace I rebased and force pushed this branch to omit the text input changes. Lemme know if it's easier to review. I'll open up a second PR the with text-input stuff after we merge this one.

@tanx tanx merged commit 62150fd into master Jan 15, 2019
@tanx tanx deleted the seed-verify-mobile-rebased branch January 15, 2019 05:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants