QR Code Scanner #4897
QR Code Scanner #4897
Conversation
|
We should be careful with colors and make sure we are following web accessibility standards for the color blind. Looks like #f1f1f1 is the lightest we should go per the us gov standards. https://designsystem.digital.gov/components/colors/ The color I used in this mock was one color code decimal off. Here's the updated at #f1f1f1. |
|
So let's make the QR button gray on hover and add a tool tip(?) for additional context since desktop webcam access to scan QR codes is a new-ish blockchain-y ux pattern. |
brunobar79
added
needs review
and removed
DO-NOT-MERGE
There was a problem hiding this comment.
Only a couple very minor blocking requests. Otherwise, my comments relate to improvements that can be made separate to this PR.
Overall, really great PR. Very solid and smooth integration with the existing codebase.
I am not able to properly QA as I don't have a working smartphone right now.
ui/lib/webcam-utils.js
Outdated
| static checkStatus () { | ||
| return new Promise((resolve, reject) => { | ||
| const isPopup = getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP | ||
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1 |
There was a problem hiding this comment.
I think isFirefox and isBrave would make sense as methods in '../../app/scripts/lib/util'
This is not a blocking request, can be done in the future.
There was a problem hiding this comment.
Good idea. It wasn't too hard so I did it on this PR.
| <div className={'qr-scanner__error'}> | ||
| {msg} | ||
| </div> | ||
| <div className={'qr-scanner__footer'}> |
There was a problem hiding this comment.
A small issue that should be fixed before the PR is merged: CANCEL and TRY AGAIN should be translated.
A slightly bigger issue that could be fixed after this PR is merged: code duplication between this <div className={'qr-scanner__footer'}> and metamask-extension/ui/app/components/page-container/page-container-footer/page-container-footer.component.js. This need not be a blocker and is really beyond the scope of this PR, but we should create a github issue (or PR) immediately after this to the needed change isn't forgotten.
There was a problem hiding this comment.
I wasn't aware of that component. Thanks for letting me know!
|
|
||
| return ( | ||
| <div className="qr-scanner"> | ||
| <div className="qr-scanner__close" onClick={this.stopAndClose}></div> |
There was a problem hiding this comment.
Not at all an issue with this PR, but we should not have to implement one of these closing 'X's for every modal. I have created a github issue to improve the modal component to provide this feature: #4956
There was a problem hiding this comment.
100% agree with you and thank you for creating the issue!
ui/app/actions.js
Outdated
| @@ -1806,6 +1832,12 @@ function hideAlert () { | |||
| } | |||
| } | |||
|
|
|||
| function qrCodeDetected (qrCodeData) { | |||
There was a problem hiding this comment.
This is a pretty small, nit-picky request, but could you make it explicit what properties are in qrCodeData? In the absence of both tests and formal documentation for this file, make properties explicit makes the code a bit more self documenting. See, for example, the showQrView, updateSendTo or setCurrentCurrency functions in this file.
In this case I am thinking of some variation of:
function qrCodeDetected ({ type, values }) {
return {
type: actions.QR_CODE_DETECTED,
value: { type, values },
}
There was a problem hiding this comment.
Hey @danjm this is a good suggestion. The main reason why I can't do that it's because I have 2 types of values: {type, values} and also null.
I need to set it to null once the view handled the code. If you have any other suggestion / workaround for that I'm happy to do it.
There was a problem hiding this comment.
Another option might be to add a docblock to the function
There was a problem hiding this comment.
Ended up following @whymarrh's suggestion. Thanks!
|
@danjm I made all the requested changes. |
Fixes #686
@alextsg @danjm @whymarrh Whenever any of you have a chance, please take a look at this PR.
Please check out this comment before looking at the code to understand the motivation behind certain parts of the code and the different scenarios that needs to be supported.
Thanks!