feat: barcode scanning #246
Conversation
🦋 Changeset detectedLatest commit: 62c1933 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
coolsoftwaretyler
changed the title
* fix(barcode-scanning): android builds * feat(barcode-scanning): wip, scanning working
* fix(barcode-scanner): initialization options * feat(barcode-scanner): add full API from MLKit for parsed types
* chore: clean up starter code in expo module * feat: working, with claude-generated swift types * fix: implement initialize method
frankcalise
left a comment
frankcalise
left a comment
There was a problem hiding this comment.
Looking good! After you review my comments and go through the Android stuff let's meet again to just align on a few of the comments - but I think it's pretty much there.
There was a problem hiding this comment.
Hm I just noticed that the directory structure here doesn't match the other modules. This one defaulted to java/expo/modules/... when we should match the other modules probably, red.infinte.reactnativemlkit.barcodescanning.
| import ExpoModulesCore | ||
| import MLKitBarcodeScanning | ||
|
|
||
| struct RNMLKitBarcodeScannerOptionsRecord: Record { |
There was a problem hiding this comment.
Should this one be named ...ScanningOptionsRecord?
I realize the Android files are Scanner in some places, probably should change that up too.
The MLKit docs reference it as BarcodeScanning so I think let's match that? Originally I had assumed it matched DocumentScanner - but my memory served me wrong and they are actually DocumentScanner and BarcodeScanning
Anyway let's discuss and align on this
There was a problem hiding this comment.
Ah yeah this was hard to keep straight, but I agree - we should align everything with the MLKit docs if we can.
There was a problem hiding this comment.
Hmm, this one is aligned with docs, I think. Google lists the options as: BarcodeScannerOptions
Working on #160. Draft for now.