-
Notifications
You must be signed in to change notification settings - Fork 17
feat(ng): ledger BTC wallet policies #476
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
base: main
Are you sure you want to change the base?
Conversation
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 had few nits and questions.nothing major! Great job!
| contentProps?: StackProps; | ||
| containerProps?: StackProps; | ||
| withViewSwitcher?: boolean; | ||
| titleProps?: TypographyProps; |
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.
titleProps seems like it is not getting used in the Page component. Do we need this one still?
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.
Nope, good catch 👌
|
|
||
| const Content = ComponentByPhase[phase]; | ||
|
|
||
| console.log('DEBUG status', status, policyName); |
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.
can we remove this log?
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.
We not only can, we need to :D Thanks!
| setStatus('idle'); | ||
| reset(); | ||
|
|
||
| if (status.endsWith(':error')) { |
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.
Since we set the status to be idle on line 33 and reset does not seem to update status, will this be ever true?
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.
Oh, I see how this can be confusing.
setStatus (like all state setters) is async, meaning that status on line 36 is the same as it was before setStatus on line 33 was called.
I'll rearrange it a bit to make it less confusing :)
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.
Ah! This change now makes sense! Thank you for updating this!
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.
🌈 🦄 Approved!
Changes
Testing
Screenshots:
Ledger.-.BTC.policy.mov
Checklist for the author