-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature/qr barcode reader #8153
Conversation
Nice one dean! One suggestion around the name of the column type, |
@Rory-Powell Yep, that's fair enough. I'll switch it out for |
Codecov Report
@@ Coverage Diff @@
## develop #8153 +/- ##
===========================================
- Coverage 67.08% 67.08% -0.01%
===========================================
Files 122 122
Lines 4123 4129 +6
Branches 658 659 +1
===========================================
+ Hits 2766 2770 +4
- Misses 950 952 +2
Partials 407 407
📣 We’re building smart automated test selection to slash your CI/CD build times. 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.
This is a really nice little component, love the UX. Great job 🚀
Nice work @deanhannigan . The UX is great and the write-up was brilliant - loved the videos. I'm with Rory on this. Scanner code is fine, but I'd prefer 'Barcode / QR code' - it is probably the most informative... but a tad long. |
@deanhannigan We should be able to use slashes in the type names anyway for the friendly names - we could go with |
…is also now listed as 'Barcode/QR Scanner'. Minor fix to include longform text columns in the table csv import list
@joebudi @shogunpurple I've updated the column type to display as When binding, the field type was previously Let me know what you think. |
@deanhannigan thanks for making the updates. Just one change - if it's not possible to have the |
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.
Great work @deanhannigan! It works perfectly and well done for getting it to work nicely with other text type fields too 👌.
Left a few comments but it's mostly minor stuff.
One other suggestion is that I'd like to move the CodeScanner
component inside the forms
directory too (beside the field), since it is not exported to use as a standalone component, and the rest of the component inside the app
directory are for usage inside apps rather than just parts of other components.
One thing which is a bit unique with this implemenation is that none of it is inside BBUI, where all our other fields are. That's fine, but it just means we can't use the code scanner field inside the builder (which is probably grand since the builder isn't mobile friendly so you'd be entering it manually anyway).
export let value | ||
export let disabled = false | ||
export let allowManualEntry = false | ||
export let scanButtonText = "Scan Code" |
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.
NAB: Change casing to "Scan code" for consistency with other buttons
<div class="modal-wrap"> | ||
<Modal bind:this={camModal} on:hide={hideReaderModal}> | ||
<ModalContent | ||
title={scanButtonText} |
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.
scanButtonText
will default to "Scan code" if the value is undefined, but if the user has entered a value in the setting field and then cleared it, the text will always be undefined as it does not account for empty strings.
2 ways to solve this would be:
$: scanButtonText = scanButtonText || "Scan code"
or every time you use scanButtonText
, do
scanButtonText || "Scan code
secondary | ||
newStyles | ||
on:click={() => { | ||
manualMode = !manualMode |
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.
NAB: since this is just a toggle, if you've enabled manual mode already and then press this again, it will unset it which feels a bit strange. I reckon we should probably just hide the button if we're already in manual mode, so {#if allowManualEntry && !manualMode}
, and then the onclick handler here can just set it explicitly to true
.
camModal.hide() | ||
}} | ||
> | ||
Enter Manually |
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.
NAB: change casing to "Enter manually" to be consistent with other buttons.
<style> | ||
#reader :global(video) { | ||
border-radius: 4px; | ||
border: var(--border-light-2); |
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.
This is a hardcoded light border so it looks off in dark themes. Let's use
border: 2px solid var(--spectrum-global-color-gray-300);
instead so that it looks better in any theme.
align-items: center; | ||
justify-content: center; | ||
border-radius: 4px; | ||
border: var(--border-light-2); |
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.
This is a hardcoded light border so it looks off in dark themes. Let's use
border: 2px solid var(--spectrum-global-color-gray-300);
instead so that it looks better in any theme.
packages/client/manifest.json
Outdated
}, | ||
{ | ||
"type": "text", | ||
"label": "Button", |
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.
NAB: let's rename this to "Button text" to make it more obvious that's what it is
packages/client/manifest.json
Outdated
"styles": [ | ||
"size" | ||
], | ||
"draggable": 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.
NAB: setting draggable to true here is redundant as it's only a flag used to disable DND if set to false. Setting illegal children is also redundant since this component doesn't take child components anyway.
Description
Adding a Code Scanner form component for scanning QR codes and a variety of barcodes.
Barcode/QR
has been added to datasources as a new column type.Barcode/QR Scanner
component is available under the 'Form' sections in the component list.Barcode/QR
will display as aBarcode/QR Scanner
in the builder and be persisted as a string.Text
orLong Form Text
orOptions
can safely switch toBarcode/QR
if requiredThe full list of supported barcodes is available here : https://github.com/mebjas/html5-qrcode
Addresses:
Screenshots
Scanning a QR Code on mobile.
Scanning a Code123 Barcode on mobile.
You can also allow manual entry on the field. This feature can be toggled in the builder
'Barcode/QR' column type db configuration. You can safely switch between all text types.
If the user hasn't granted camera permissions on their device.