Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Code Review for Job #18413 #149

Merged
merged 41 commits into from Nov 16, 2012

Conversation

Projects
None yet
2 participants
Owner

birarda commented Nov 15, 2012

Code Review for Job #18413 - Workitem available at https://www.worklist.net/worklist/workitem.php?job_id=18413&action=view

birarda added some commits Nov 13, 2012

@birarda birarda use apple maps for directions if iOS 6 8c0def0
@birarda birarda add splash for i5 support, bring back bounce in resume e112083
@birarda birarda fix autoresizing of height of table view in check in list 8a1c114
@birarda birarda fix autoresizing of map on check in details screen 311a60f
@birarda birarda fix map marker position on check in details map e7ea41c
@birarda birarda use helper method for map shifts 9cfa9c6
@birarda birarda conditional text offset, macro file for common macros 59a4238
@birarda birarda fix resizing of skills table view in love VC 7b56222
@birarda birarda fix chat entry field positioning for one-on-one chat a3b4548
@birarda birarda fix hiding/showing of placeholder image to contacts TVC 9de3adc
@birarda birarda add helper to CPBaseTVC class for tab bar button avoiding view 43b5fcd
@birarda birarda add tab bar button avoiding footer view in venue and user TVCs 0bb5405
@birarda birarda abandon SVProgressHUD for SVPullToRefresh in UserListTVC e95e899
@birarda birarda don't give directions to neighborhood venues 7d17d51
@birarda birarda force pull to refresh animation only on first load 7d46dcc
@birarda birarda autoresize resume paper to match behavior on 3.5" devices 223ffee
@birarda birarda add iPhone 5 splash that matches the 3.5 inch one e91eddf
@birarda birarda don't hide contact list search bar for four inch device 5c05ccb
@birarda birarda show larger tutorial images for 4 inch device cacb949
@birarda birarda rename old school iPad friendly images 3cab0ce
@birarda birarda fix subview layout and resume view size d227629
@birarda birarda show larger splash for signup on 4 inch device 6414368
@birarda birarda move appropriate calls back to viewDidLoad 7f65909
@birarda birarda don't need viewDidLayoutSubviews in SignupController 1c1fa55
@birarda birarda more shuffling of calls to viewDidLoad instead of viewDidLayoutSubviews 643001d

@heracek heracek and 1 other commented on an outdated diff Nov 15, 2012

candpiosapp/Checkin/CheckInDetailsViewController.m
@@ -529,4 +532,8 @@ - (void)dismissViewControllerAnimated {
[self dismissModalViewControllerAnimated:YES];
}
+- (void)viewDidUnload {
+ [self setMapPinImageView:nil];
@heracek

heracek Nov 15, 2012

Contributor

please use propery notation: self.mapPinImageView = nil

@birarda

birarda Nov 15, 2012

Owner

actually this auto-generated call to nil out the outlet is useless. removing alltogether.

@heracek heracek commented on an outdated diff Nov 15, 2012

candpiosapp/Common/CPBaseTableViewController.m
@@ -86,6 +86,12 @@ - (void) viewDidAppear:(BOOL)animated {
[self unhighlightCells:YES];
}
+- (UIView *)tabBarButtonAvoidingFooterView
+{
+ UIView *avoidingFooter = [[UIView alloc] initWithFrame:CGRectMake(0, 0, self.tableView.frame.size.width, [[CPAppDelegate tabBarController].thinBar actionButtonRadius])];
+ return avoidingFooter;
@heracek

heracek Nov 15, 2012

Contributor

no need to assign to variable, just return it.

line is too long, try splitting on separate lines:

CGRectMake(0,
           0,
           ...,
           ...)

@heracek heracek commented on an outdated diff Nov 15, 2012

candpiosapp/Common/CPMacros.h
+// Copyright (c) 2012 Coffee and Power Inc. All rights reserved.
+//
+
+#ifndef candpiosapp_CPMacro_h
+#define candpiosapp_CPMacro_h
+
+// redefine NSLog so it throws to TF and prints it all pretty
+#define NSLog(__FORMAT__, ...) TFLog((@"%s [Line %d] " __FORMAT__), __PRETTY_FUNCTION__, __LINE__, ##__VA_ARGS__)
+
+// define a way to quickly grab the app delegate
+#define CPAppDelegate (AppDelegate *)[UIApplication sharedApplication].delegate
+
+// quick way to check iOS version
+#define SYSTEM_VERSION_GREATER_THAN_OR_EQUAL_TO(v) ([[[UIDevice currentDevice] systemVersion] floatValue] >= v)
+
+#define IS_DEVICE_WITH_FOUR_INCH_DISPLAY (UI_USER_INTERFACE_IDIOM() == UIUserInterfaceIdiomPhone && [[UIScreen mainScreen] bounds].size.height == 568)
@heracek

heracek Nov 15, 2012

Contributor

macros should be avoided, it's good idea for NSLog, but rest could be implemented using function calls (it's more secure and allows future extension)

@heracek heracek commented on an outdated diff Nov 15, 2012

candpiosapp/Contacts/ContactListViewController.m
@@ -193,12 +193,18 @@ - (void)setContacts:(NSMutableArray *)contactList {
self.sortedContactList = [contactList mutableCopy];
}
-- (void)hidePlaceholder:(BOOL)hide
+- (void)togglePlaceholder:(BOOL)hiddenPlaceholder
@heracek

heracek Nov 15, 2012

Contributor

toggle is good name if there is no argument, but when there is hidden argument it's not that obvious what this means: [self togglePlaceholder:YES].
Previous method name gives more meaning, or setPlaceholderHidden: ([UIVies setHidden:]) could be better too.

@heracek heracek and 1 other commented on an outdated diff Nov 15, 2012

candpiosapp/Contacts/ContactListViewController.m
{
- [self.placeholderImage setHidden:hide];
- [self.tableView setScrollEnabled:hide];
- [self.searchBar setHidden:!hide];
- self.isSearching = !hide;
+ if (!hiddenPlaceholder && !self.placeholderImageView) {
+ self.placeholderImageView = [[UIImageView alloc] initWithImage:[UIImage imageNamed:@"contacts-blank-slate"]];
+ }
+
+ self.tableView.tableFooterView = hiddenPlaceholder ?
+ [self tabBarButtonAvoidingFooterView] :
+ self.placeholderImageView;
@heracek

heracek Nov 15, 2012

Contributor

is indention right here?

@birarda

birarda Nov 15, 2012

Owner

Not sure what the standard is for ternary operator indentation.

@birarda

birarda Nov 15, 2012

Owner

Looking into it.

@heracek heracek and 1 other commented on an outdated diff Nov 15, 2012

candpiosapp/Login/SignupController.m
@@ -10,6 +10,17 @@
#import "Flurry.h"
#import "UIViewController+isModal.h"
+@interface SignupController()
+
+- (IBAction)loginWithLinkedInTapped:(id)sender;
+- (IBAction) dismissClick:(id)sender;
@heracek

heracek Nov 15, 2012

Contributor

there should be no space after )

no need to declare IBAction, Interface Builder will take it from declaration in @implementation section (just use IBAction there)

@birarda

birarda Nov 15, 2012

Owner

I didn't declare this, copy paste from header to make them private. will remove.

@heracek heracek commented on an outdated diff Nov 15, 2012

candpiosapp/User/UserProfileViewController.m
+ }];
+ }
+}
+
+- (void)viewDidLayoutSubviews
+{
+ [super viewDidLayoutSubviews];
+
+ // when pulling the scroll view top down, present the map
+ self.mapView.frame = CGRectUnion(self.mapView.frame,
+ CGRectOffset(self.mapView.frame, 0, -self.mapView.frame.size.height));
+
+ // add the blue overlay gradient in front of the map
+ [self addGradientWithFrame:self.mapView.frame
+ locations:@[[NSNumber numberWithFloat:0.25],
+ [NSNumber numberWithFloat:0.30],
@heracek

heracek Nov 15, 2012

Contributor

please indent on same level as first NSNumber

@heracek heracek commented on an outdated diff Nov 15, 2012

candpiosapp/User/UserProfileViewController.m
+ // when pulling the scroll view top down, present the map
+ self.mapView.frame = CGRectUnion(self.mapView.frame,
+ CGRectOffset(self.mapView.frame, 0, -self.mapView.frame.size.height));
+
+ // add the blue overlay gradient in front of the map
+ [self addGradientWithFrame:self.mapView.frame
+ locations:@[[NSNumber numberWithFloat:0.25],
+ [NSNumber numberWithFloat:0.30],
+ [NSNumber numberWithFloat:0.5],
+ [NSNumber numberWithFloat:0.90],
+ [NSNumber numberWithFloat:1.0]]
+ colors:@[(id)[[UIColor colorWithRed:0.67 green:0.83 blue:0.94 alpha:1.0] CGColor],
+ (id)[[UIColor colorWithRed:0.67 green:0.83 blue:0.94 alpha:0.75] CGColor],
+ (id)[[UIColor colorWithRed:0.40 green:0.62 blue:0.64 alpha:0.4] CGColor],
+ (id)[[UIColor colorWithRed:0.67 green:0.83 blue:0.94 alpha:0.75] CGColor],
+ (id)[[UIColor colorWithRed:0.67 green:0.83 blue:0.94 alpha:1.0] CGColor]]
@heracek

heracek Nov 15, 2012

Contributor

please indent on same level as first

@heracek heracek added a commit that referenced this pull request Nov 16, 2012

@heracek heracek Merge pull request #149 from birarda/18413
Code Review for Job #18413
1ec726b

@heracek heracek merged commit 1ec726b into highfidelity:master Nov 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment