webserver/ui: Generate QR codes for deposit address #1483
Conversation
|
|
client/webserver/http.go
Outdated
| if darkMode { | ||
| qr.BackgroundColor = color.Black | ||
| qr.ForegroundColor = color.White | ||
| } else { | ||
| qr.BackgroundColor = color.White | ||
| qr.ForegroundColor = color.Black | ||
| } |
There was a problem hiding this comment.
I have tried this with two scanners, and one is fine, but one cannot pick up the white qr code on black background. Can they always be white background with black foreground?
There was a problem hiding this comment.
Well, I tried both qr code on two scanners and they work well. Maybe scanner related issue?
There was a problem hiding this comment.
They both work for me too. If it turns out to be a more widespread issue, we can have the light mode version always displayed by using a white/light patch behind it.
There was a problem hiding this comment.
They both work fine for me too. In the QR code library's docs there was an example with a black background and white foreground so I thought it would be fine, but now I'm doing some research and it's recommended that the foreground should be darker than the background.
There was a problem hiding this comment.
Actually it might even look better.
|
An error unrelated to the PR is causing the failed checks. |
|
Restarted the CI run. It was having trouble fetching the linter itself. :P |
There was a problem hiding this comment.
LGTM.
I have some slight concerns about potential abuse of the route if the service or it's host are not secured properly. e.g. With the /generateqrcode page not in a requireLogin router group, it could be called by anyone with network access to the listening port. There's no sensitive info as with the wallets page, but it consumes cpu and memory resources to do the qr and png encoding.
A related concern is a max length of the address string. A long enough string could be trouble for the qr encoder. Not tested though.
Given the default to listen only on a loopback address, I'm not requesting any changes though. Just something we should keep in mind.
Closes #1449