-
Notifications
You must be signed in to change notification settings - Fork 368
ctb-ios-changes #78
base: master
Are you sure you want to change the base?
ctb-ios-changes #78
Conversation
@josharian or @dgoldman-ebay, do you have any feedback on this PR? |
@@ -1016,7 +1016,7 @@ + (BOOL)cardExpiryIsValid:(CardIOCreditCardInfo*)info { | |||
} | |||
|
|||
// we are under the assumption of a normal US calendar | |||
NSCalendar *cal = [[NSCalendar alloc] initWithCalendarIdentifier:NSCalendarIdentifierGregorian]; | |||
NSCalendar *cal = [[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar]; |
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.
No. NSCalendarIdentifierGregorian
was correct. See https://developer.apple.com/reference/foundation/nsgregoriancalendar?language=objc
if(!result.usable) { | ||
|
||
if(!result.usable) | ||
{ |
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.
The style used within this codebase is to keep opening braces on the previous line. Please stick to this style. (Doesn't apply, of course, when you bring whole new libraries into the project.)
@@ -504,7 +504,7 @@ - (void)cardIOView:(CardIOView *)cardIOView didScanCard:(CardIOCreditCardInfo *) | |||
dataEntryViewController.manualEntry = self.context.suppressScannedCardImage; | |||
|
|||
CGPoint newCenter = [self.view convertPoint:cardIOView.transitionView.cardView.center fromView:cardIOView.transitionView]; | |||
newCenter.y -= NavigationBarHeightForOrientation([[UIApplication sharedApplication] statusBarOrientation]); | |||
newCenter.y -= NavigationBarHeightForOrientation(self.interfaceOrientation); |
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.
Orientation stuff is tricky and often runs into issues with unexpected edge cases. I hope that you've tried every possible experiment!
I've added a few extremely minor comments. Otherwise, it looks like this huge PR mostly comprises a couple of large libraries, on which I really can't comment. I hope that they don't increase the shipped size of card.io terribly much... At first, you might want to branch card.io, with a new branch that incorporates these and future related changes, and make both versions of card.io available to clients. |
See most recent Readme for description: https://github.com/CTBConsulting/Card.io-for-Capital-One-vertical-Cards/blob/dev/README.md