-
Notifications
You must be signed in to change notification settings - Fork 387
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
Swift 4 #64
Swift 4 #64
Conversation
Target iOS 9.3. Add support for UPC-A. Add all new barcode types to the metadata array.
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.
I like this PR, looks great @chadcummings!
@@ -343,8 +343,6 @@ open class BarcodeScannerController: UIViewController { | |||
center(subview: focusView, inSize: CGSize(width: 218, height: 150)) | |||
} | |||
center(subview: settingsButton, inSize: CGSize(width: 150, height: 50)) | |||
|
|||
headerView.isHidden = !isBeingPresented |
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.
@chadcummings What is the reason behind this change?
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 fixes a bug when the screen orientation was changed, the Header would disappear and never reappear, even when the device was rotated back. I haven't noticed any negative side effects, but I'll look to you for guidance here.
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.
I think it was here for the case when you have UINavigationController
and push barcode scanner controller, so you don't end up with two "navigation bars". But arguably it's something you can manually control in the 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.
I'll see if I can test this out by enhancing the example app to verify. I'll fix it if necessary.
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.
@chadcummings Keep us posted. It's the only thing we'd like to verify before merging this PR.
@@ -228,7 +234,7 @@ | |||
GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; | |||
GCC_WARN_UNUSED_FUNCTION = YES; | |||
GCC_WARN_UNUSED_VARIABLE = YES; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.2; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.3; |
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.
Could it be 9.0
or there are some specific APIs you want to use? @chadcummings
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.
After reviewing which devices can run iOS 9.0, I discovered that all of the target devices can also run iOS 9.3. This led me to the conclusion that is was pointless to support iOS 9.0 when all devices that run iOS 9.0 will run iOS 9.3. Supporting iOS 9.0 has no benefit, as no additional devices are gained. That's my only reasoning, really.
Ultimately, this code is your's. I am happy to use whichever version you would like, but I would prefer trimming extra baggage as much as possible.
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.
I totally agree with you, but I think we still have projects with minimum target set to 9.0, which is probably something we might want to change. What do you think @onmyway133 @zenangst?
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.
I don't have strong opinion about this. But as there's no reason to raise to 9.3, then 9.0 is fine I think
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.
Sounds good. I'll move it to 9.0.
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.
@vadymmarkov yeah we should totally change that. But if we don't need 9.3
changes in this PR then it should still use 9.0
:)
great @chadcummings |
I like this PR! |
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.
Sorry, everyone. Work got a little busy for a bit. I've changed the target to 9.0 and that all works well. I enhanced the example project to demonstrate push and present navigation to ensure we didn't have a problem with "double navigation bars" with a line of code I removed. I'll see if I can get that fixed quick so we can finish up this PR. |
Ooookkaaayyy, That's embarrassing. Apparently my iPad physically doesn't have a a light with the camera. Which explains why the flashButton doesn't show up. Problem solved. |
Enhance Example project to include buttons for push navigation and presenting the scanner.
Final fixes done. Let me know if there is anything else to do. Thanks. |
@chadcummings Awesome! Thanks for your contribution. |
Version 3.0.0 is out. |
Swift 4 Upgrades! Let me know if I missed anything.