-
Notifications
You must be signed in to change notification settings - Fork 169
Conversation
fe59dba
to
a11ad95
Compare
a11ad95
to
e5ce7e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the way these screens turned out :) I pushed a small change that allows navigation to Home after pin setup, but if your PR that builds on top of this already does that feel free to remove the commit.
Seems like mobile is really coming together! :D
|
||
const SetPasswordConfirmView = ({ store, wallet }) => ( | ||
<Background image="purple-gradient-bg"> | ||
<MainContent style={styles.content}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we either add a back button so the user can nav back to the previous screen, or open an issue to add one in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you hit the backspace
button when the pin bubbles are empty it will already navigate back ;) #UXhacks
@@ -202,11 +240,11 @@ class WalletAction { | |||
* there was no typo. | |||
* @return {Promise<undefined>} | |||
*/ | |||
async checkNewPassword() { | |||
async checkNewPassword(minLength = MIN_PASSWORD_LENGTH) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we'll have error message notifications if the PINs don't match when #603 is closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah notification aren't wired up yet. Currently working on #607 next to handle PIN screen errors.
}, | ||
}); | ||
|
||
export const PinBubbles = ({ pin }) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mobile UI is currently only tested on phone resolutions. I'll share screens in app-dev.
const { wallet } = this._store; | ||
if (wallet[param]) { | ||
wallet[param] = wallet[param].slice(0, -1); | ||
} else if (param === 'passwordVerify') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a back
button might be better, it's a little unintuitive to me that I can delete my way back to the previous screen? Also wouldn't this be a problem if the user is just logging in normally, not setting a new password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unlock screen is a different view implemented in password-mobile.js
... coming up on the next branch. I agree we could add a back button. That would be a bit of a deviation from the current designs though and probably better to do that in another PR.
this.checkNewPassword(PIN_LENGTH); | ||
} else if (param === 'password') { | ||
this.checkPassword(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff
}, | ||
}); | ||
|
||
export const PinKeyboard = ({ onInput, onBackspace }) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how this turned out visually :) It's a bit slow for me on entry/backspace, but that might just be expo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test on Android? Currently only testing on iOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah only on Android, if it's fast enough on iOS that should be fine
@@ -113,6 +113,53 @@ describe('Action Wallet Unit Tests', () => { | |||
}); | |||
}); | |||
|
|||
describe('pushPinDigit()', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
info={{ initLoaderSyncing: () => nav.goHome() }} | ||
/> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valentinewallace noice! 🙏
@valentinewallace thanks. The change looks good to me. I agree with your comments. Those should all be addressed in future PRs. |
Closes #577, closes #578