ui: restyle wallets page - #1700
Conversation
df6fb0d to
752dfc1
Compare
d3bb6e7 to
6ec03b5
Compare
e8eb16b to
e6aa794
Compare
| console.log(note) | ||
| console.log(error.stack) |
There was a problem hiding this comment.
leftovers or you want this because it shouldn't happen?
| Doc.hide(page.sendErr) | ||
| const assetID = parseInt(page.sendForm.dataset.assetID || '') | ||
| const subtract = page.subtractCheckBox.checked || false | ||
| const assetID = parseInt(page.sendForm.dataset.assetID ?? '') |
There was a problem hiding this comment.
Eeeek have we broadly misusing || when we should have been using the nullish coalescing operator?
There was a problem hiding this comment.
Meh. I just discovered this, really. Will try to use it more in the future, but it's already bitten me. I did something like
const v = parseFloat(inputElement.value ?? '0')which yields NaN, because value is not undefined or null, just empty, which is falsy but not nullish.
I think we should use it, but I'm not going to enforce it too strictly.
I've also been using the optional chaining operator, ?..
const thing = { a: 1 }
let v = thing.b.c // error thrown cuz b undefined
v = thing?.b?.c // undefined
chappjc
left a comment
There was a problem hiding this comment.
Tests well overall, just a few minor things.
And the following is not an issue with this PR, but there's inconsistent terminology with the built-in wallets. We call them "Native" for DCR and BTC, but "Internal" for ETH:
This is just something we don't want the user scratching their head over.
Also in that BTC screen, it's a whole number so there are no decimals, not even a single ".0". I guess that's fine if intended, but looks a little unusual for wallet displays.
|
Thanks for the review followup. Just want to make sure you saw that I had a prior review: #1700 (review) No rush but I got the sense you just were only addressing the second review from my testing. The order filled percent is the main outstanding thing. |
chappjc
left a comment
There was a problem hiding this comment.
OK. I thought we could just capitalize the other languages where it looks safe, but whatever
The wallets page is restyled. The new style is responsive.
Desktop
Tablet
Mobile
