Skip to content
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

Ios init #207

Closed
wants to merge 5 commits into from
Closed

Ios init #207

wants to merge 5 commits into from

Conversation

dhanya
Copy link
Contributor

@dhanya dhanya commented Oct 6, 2013

#39 and #40 UI fixes

@@ -31,58 +31,85 @@ @implementation PacoConsentViewController
@synthesize experiment = _experiment;

+ (PacoConsentViewController*)controllerWithExperiment:(PacoExperimentDefinition *)experiment {
PacoConsentViewController* controller =
PacoConsentViewController* controller =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use 2 spaces as the indent, and use spaces instead of tabs. Please don't change this style. And any new code should follow this style too. You can set up the Xcode by going to Preferences, Text Editing, Indentation, then
a. set "Prefer indent using" to Spaces.
b. set both Tab width and Indent width to 2 spaces.

For detailed coding style, check Google's Objective-C style guide:
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml

@yimingcheung
Copy link
Contributor

Please clean this pull request by using 2 spaces for the indentation.

frame.origin.y = 420 - (frame.size.height / 2) - 25;
accept.frame = frame;
[super viewDidLoad];
if ([self respondsToSelector:@selector(edgesForExtendedLayout)])//for ios7, to adjust layout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although there is only one line in the if-block, it's error-prone to miss the brackets. Please add the brackets:
if ([self respondsToSelector:@selector(edgesForExtendedLayout)]) { //for ios7, to adjust layout
self.edgesForExtendedLayout = UIRectEdgeNone;
}

@yimingcheung
Copy link
Contributor

Thanks for the code @dhanya !
Code review is finished, please fix all the commented issues and ping me when you are done, so that I can merge this fix.
By the way, could you also please attach a screenshot of each fix to the tickets so that it's clear how these two UI bugs fixed, thanks!

@dhanya
Copy link
Contributor Author

dhanya commented Oct 9, 2013

Attached screenshots for the fixes. They are iOS 6 screenshots that i had, as i haven't been able to get the app to load on the phone running ios 7 as the server is down.

#53
ios simulator screen shot 09-oct-2013 6 41 02 pm
#56
ios simulator screen shot 09-oct-2013 6 41 46 pm
#47
ios simulator screen shot 09-oct-2013 6 41 54 pm

@ghost ghost assigned yimingcheung Oct 10, 2013
@@ -35,13 +35,13 @@ - (void)application:(UIApplication *)application didReceiveLocalNotification:(UI
if (notification == nil) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't commit this kind of change, it's not necessary, and makes the code difficult to review.

@dhanya dhanya deleted the ios-init branch October 10, 2013 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants