feat(qr): add placeholder QR code scanner #19679
Conversation
jrainville
requested review from
a team,
alexjba,
caybro,
micieslak and
noeliaSD
as code owners
jrainville
force-pushed
the
feat/qr-code-scanner-placeholder
branch
from
291d932 to
ef78327
Compare
Fixes #19677 Adds a placeholder button to open a QR code scanner dialog. The dialog itself follows the design, though it might be outdated.
jrainville
force-pushed
the
feat/qr-code-scanner-placeholder
branch
from
ef78327 to
19660fb
Compare
Jenkins BuildsClick to see older builds (13)
|
caybro
left a comment
caybro
left a comment
There was a problem hiding this comment.
LGTM overall, some suggestions to make the new component more flexible
| property alias cameraWidth: cameraLoader.width | ||
|
|
||
| signal connectionStringFound(connectionString: string) | ||
| signal validTagFound(tag: string) |
There was a problem hiding this comment.
| signal validTagFound(tag: string) | |
| signal validTagFound(string tag) |
Better follow the usual signal signature pattern; eventhough it seems the extended JS syntax is also supported here
|
|
||
| StatusBaseText { | ||
| visible: d.showCamera && cameraLoader.item.currentTag ? true : false | ||
| visible: d.showCamera && !!d.errorMessage |
There was a problem hiding this comment.
| visible: d.showCamera && !!d.errorMessage | |
| visible: d.showCamera && !!text |
| errorMessage: qsTr("Status doesn't understand the QR code.") | ||
| validate: function (tag) { | ||
| // We accept URLs and addresses | ||
| return Utils.isURL(tag) || Utils.isValidAddress(tag) |
There was a problem hiding this comment.
Wouldn't it be better to pass this validator function from the outside? For better flexibility
There was a problem hiding this comment.
This component is meant to be the one meant to be used to scan addresses and URLs by the user, so it's not meant to be generic. The generic scanner is StatusQRCodeScanner and it already has an outside validator.
I think putting the logic in this component makes sense, because it is its own area of concern.
| id: d | ||
|
|
||
| property string validTag: "" | ||
| property bool validTagFound: false |
There was a problem hiding this comment.
The boolean redundant? If validTag !== "" then we know we found something :)
| if (Utils.isURL(d.validTag)) { | ||
| root.urlScanned(d.validTag) | ||
| } else if (Utils.isValidAddress(d.validTag)) { | ||
| root.addressScanned(d.validTag) |
There was a problem hiding this comment.
Hmm, maybe the external validator function wouldn't work here with the dedicated signals approach... what about having a toplevel enum for the recognized/supported tag types (easy to extend) and a signal tagFound(int tagType, string tag); wdyt?
|
|
||
| footer: Item { | ||
| visible: false | ||
| } |
There was a problem hiding this comment.
Uhm, what's the purpose of this? :)
There was a problem hiding this comment.
To hide the footer 🫣
I don't know what's the clean way to do that
There was a problem hiding this comment.
Yup, it was that simple 🤦
|
Thanks for the review @caybro . I addressed your comments |
There was a problem hiding this comment.
I'm not sure why, but it seems like I broke the translations here @caybro
3 participants
What does the PR do
Fixes #19677
Adds a placeholder button to open a QR code scanner dialog. The dialog itself follows the design, though it might be outdated.
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
User profile URL:
user-url.webm
Address:
address-scan.webm
Random URL:
random-site.webm
Impact on end user
Adds a useful feature, especially on mobile
How to test
Risk
Low. Worst case it doesn't scan well