-
Notifications
You must be signed in to change notification settings - Fork 169
Conversation
@valentinewallace also here please link the issue ... it's easier to follow the links when reviewing. Thanks :) |
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'll wait with an in depth review until after we merge the seed-mobile
PR. Just one thing that I noticed on first pass.
src/action/auth-mobile.js
Outdated
return; | ||
} | ||
this.initSetPin(); | ||
} |
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 auth controller is only for pin entry and fingerprint. Seed stuff should go in a different controller. Any reason we can't reuse the logic from the desktop?
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.
Fixed!
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.
Seems like there is still seed logic in auth-mobile.js... maybe you forgot to push something?
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.
My mistake, fixed! Thought that only referred to checkSeed
for some reason.
dd7cf60
to
345587d
Compare
dbe8610
to
7385ba8
Compare
This will allow mobile views with non-capslock'ed Titles to still use the Title component.
7385ba8
to
ebe7e0e
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.
@valentinewallace ah I just saw this PR includes both the seed-mobile and seed-verify-mobile views. My feedback from #799 applies here too. Maybe we should review #799 first and then get to this one?
|
Closing in favor of #878 |
Closes #582 and #581.