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

Add vertical scale answer format #33

Merged
merged 25 commits into from Apr 21, 2015

Conversation

Projects
None yet
5 participants
@rsanchezsaez
Contributor

rsanchezsaez commented Apr 15, 2015

I have implemented vertical scale support (issue #3).

ORKScaleSlider now has the isVertical property. When set to YES, it internally uses the UIView'stransform property and some minor bounds and touch trickery to draw and work in vertical.

ORKScaleSliderView now has vertical support through the use of the new isVertical method from the ORKScaleAnswerFormatProvider.

New classes: ORKVerticalScaleAnswerFormat and ORKContinuousVerticalScaleAnswerFormat. The corresponding example steps have been added to ORKCatalog's Scale Question.

rsanchezsaez added some commits Apr 15, 2015

ORKScaleSlider: fix issue when left dragging touch outside the segmen…
…ted scale

(Scale was being set to the maximumValue when you dragged the touch outside the segmented scale through the view's left bound)
ORKAnswerFormat: add ORKVerticalScaleAnswerFormat and ORKContinuousVe…
…rticalScaleAnswerFormat classes and factory methods

Also:
ORKAnswerFormat .h: make spacing coherent; add missing ORKContinuousScaleAnswerFormat initializer comment
TaskListRow: add DiscreteVerticalScaleQuestionStep and ContinuousVert…
…icalScaleQuestionStep examples to scaleQuestionTask
@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 15, 2015

Contributor

I tried to visually layout vertical scales so the scrolling is minimum on iPhone 4S sizes, but this produces somewhat short vertical scales. Maybe we should layout to iPhone 5 sizes and just have iPhone 4S users scrolling a bit.

Also, I'm not happy by the fact I had to increase the ORKSurveyAnswerCellForScale's suggestedCellHeightForView: (shared by both vertical and horizontal scales) so it fits the vertical ones.

Ideally, the height wouldn't increase for the horizontal scale cell (and vertical ones could be made even taller). But I couldn't figure a simple and sound way of doing this without a major refactor (since the ORKSurveyAnswerCellForScale class is inferred from ORKQuestionTypeScale, and it doesn't make much sense to add an additional ORKQuestionTypeVerticalScale value just for the vertical scales). Any suggestion regarding this issue?

Contributor

rsanchezsaez commented Apr 15, 2015

I tried to visually layout vertical scales so the scrolling is minimum on iPhone 4S sizes, but this produces somewhat short vertical scales. Maybe we should layout to iPhone 5 sizes and just have iPhone 4S users scrolling a bit.

Also, I'm not happy by the fact I had to increase the ORKSurveyAnswerCellForScale's suggestedCellHeightForView: (shared by both vertical and horizontal scales) so it fits the vertical ones.

Ideally, the height wouldn't increase for the horizontal scale cell (and vertical ones could be made even taller). But I couldn't figure a simple and sound way of doing this without a major refactor (since the ORKSurveyAnswerCellForScale class is inferred from ORKQuestionTypeScale, and it doesn't make much sense to add an additional ORKQuestionTypeVerticalScale value just for the vertical scales). Any suggestion regarding this issue?

Show outdated Hide outdated ResearchKit/Common/ORKAnswerFormat.h
ORK_CLASS_AVAILABLE
@interface ORKVerticalScaleAnswerFormat : ORKScaleAnswerFormat
@end

This comment has been minimized.

@YuanZhu-apple

YuanZhu-apple Apr 15, 2015

Member

Would it be much simpler to just add a vertical boolean property on ORKScaleAnswerFormat and ORKContinuousScaleAnswerFormat?

@YuanZhu-apple

YuanZhu-apple Apr 15, 2015

Member

Would it be much simpler to just add a vertical boolean property on ORKScaleAnswerFormat and ORKContinuousScaleAnswerFormat?

Show outdated Hide outdated ResearchKit/Common/ORKSurveyAnswerCellForScale.m
@@ -119,9 +119,7 @@ - (IBAction)sliderValueChanged:(id)sender {
}
+ (CGFloat)suggestedCellHeightForView:(UIView *)view {
return 140.0;
return 244.0;
}

This comment has been minimized.

@YuanZhu-apple

YuanZhu-apple Apr 15, 2015

Member

I think better to add a second parameter isVertical to this method, so it can return different height for different layout. The height wouldn't increase for the horizontal scale cell.

@YuanZhu-apple

YuanZhu-apple Apr 15, 2015

Member

I think better to add a second parameter isVertical to this method, so it can return different height for different layout. The height wouldn't increase for the horizontal scale cell.

This comment has been minimized.

@jwe-apple

jwe-apple Apr 15, 2015

Member

Yuan - not sure quite how that would work! This is a class method on the base class, and vertical/horizontal is very specific to this class. It might be easiest to just create a subclass of ORKSurveyAnswerCellForScale to cover the vertical case.

@jwe-apple

jwe-apple Apr 15, 2015

Member

Yuan - not sure quite how that would work! This is a class method on the base class, and vertical/horizontal is very specific to this class. It might be easiest to just create a subclass of ORKSurveyAnswerCellForScale to cover the vertical case.

@YuanZhu-apple

This comment has been minimized.

Show comment
Hide comment
@YuanZhu-apple

YuanZhu-apple Apr 15, 2015

Member

Congratulations on the FIRST new feature PR for ResearchKit!

To the visually layout of vertical scale, scale bar should has an adequate height to assist user to select on the bar easier.

  • Value labels should be move to the left to save space for scale bar
  • It's OK to have a fixed height across different screen sizes, but it is better to make the page is not scrollable on iPhone 6 and iPhone 6 plus (page scrolling on iphone 4s and iphone 5 is fine). Try this magic cell height: 358.0.

I did an engineering mockup:
mockup

Member

YuanZhu-apple commented Apr 15, 2015

Congratulations on the FIRST new feature PR for ResearchKit!

To the visually layout of vertical scale, scale bar should has an adequate height to assist user to select on the bar easier.

  • Value labels should be move to the left to save space for scale bar
  • It's OK to have a fixed height across different screen sizes, but it is better to make the page is not scrollable on iPhone 6 and iPhone 6 plus (page scrolling on iphone 4s and iphone 5 is fine). Try this magic cell height: 358.0.

I did an engineering mockup:
mockup

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 15, 2015

Member

One more comment - to accept this pull request, it will also need to work properly with VoiceOver enabled. The existing horizontal control has VoiceOver support, so this should be straightforward.

Member

jwe-apple commented Apr 15, 2015

One more comment - to accept this pull request, it will also need to work properly with VoiceOver enabled. The existing horizontal control has VoiceOver support, so this should be straightforward.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 15, 2015

Contributor

Hi guys,

thanks for the feedback! I'll look into the design changes. Definitely the range labels on the side of the slider is a good idea that would save some vertical space.

I tried displaying _valueLabel to the left of the vertical scale before but it didn't look quite right (all the rest of the answer formats have a nice vertical-axis hierarchical organisation to their UI elements, and I thought this setup kind of broke it). I'll try again and report back.

Regarding the height of ORKSurveyAnswerCellForScale, I had the problem that @jwe-apple mentions: it's quite tricky to send parameters to the suggestedCellHeightForView: method without a big refactor. Creating a ORKSurveyAnswerCellForVerticalScale subclass may be the most straightforward solution. Note that I'll have to add an (otherwise needless) ORKQuestionTypeVerticalScale value for this to work.

Regarding adding an isVertical property to ORKScaleAnswerFormat and ORKContinuousScaleAnswerFormat instead of adding the ORKVerticalScaleAnswerFormat and ORKContinuousVerticalScaleAnswerFormat subclasses, I thought the additional classes in my implementation were neater from the framework user perspective (and would allow for future, better encapsulated, visual customization of the subclasses, without polluting the superclasses). But tell me if actually adding the property makes more sense and I'll modify that, this should be pretty straight forward.

Contributor

rsanchezsaez commented Apr 15, 2015

Hi guys,

thanks for the feedback! I'll look into the design changes. Definitely the range labels on the side of the slider is a good idea that would save some vertical space.

I tried displaying _valueLabel to the left of the vertical scale before but it didn't look quite right (all the rest of the answer formats have a nice vertical-axis hierarchical organisation to their UI elements, and I thought this setup kind of broke it). I'll try again and report back.

Regarding the height of ORKSurveyAnswerCellForScale, I had the problem that @jwe-apple mentions: it's quite tricky to send parameters to the suggestedCellHeightForView: method without a big refactor. Creating a ORKSurveyAnswerCellForVerticalScale subclass may be the most straightforward solution. Note that I'll have to add an (otherwise needless) ORKQuestionTypeVerticalScale value for this to work.

Regarding adding an isVertical property to ORKScaleAnswerFormat and ORKContinuousScaleAnswerFormat instead of adding the ORKVerticalScaleAnswerFormat and ORKContinuousVerticalScaleAnswerFormat subclasses, I thought the additional classes in my implementation were neater from the framework user perspective (and would allow for future, better encapsulated, visual customization of the subclasses, without polluting the superclasses). But tell me if actually adding the property makes more sense and I'll modify that, this should be pretty straight forward.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 15, 2015

Member

We've found the answer formats tend to be pretty static and think a property makes sense. Probably read-only, but could be convinced otherwise.

Member

jwe-apple commented Apr 15, 2015

We've found the answer formats tend to be pretty static and think a property makes sense. Probably read-only, but could be convinced otherwise.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 16, 2015

Contributor

A read-only property passed to the answer format initializers makes sense. I'll make the changes soon.

Do you prefer isVertical or vertical for the property names (for ORKScaleSlider, ORKScaleAnswerFormatProvider, ORKScaleAnswerFormat, and ORKContinuousScaleAnswerFormat)? Looking at past usages in other Apple code: iOS' UITextSelectionRect uses isVertical, but OS X's NSSplitView uses vertical.

Coding Guidelines for Cocoa naming conventions seem to recommend using the vertical name with a custom specified isVertical get accessor as in:

@property (assign, getter=isEditable) BOOL editable;
Contributor

rsanchezsaez commented Apr 16, 2015

A read-only property passed to the answer format initializers makes sense. I'll make the changes soon.

Do you prefer isVertical or vertical for the property names (for ORKScaleSlider, ORKScaleAnswerFormatProvider, ORKScaleAnswerFormat, and ORKContinuousScaleAnswerFormat)? Looking at past usages in other Apple code: iOS' UITextSelectionRect uses isVertical, but OS X's NSSplitView uses vertical.

Coding Guidelines for Cocoa naming conventions seem to recommend using the vertical name with a custom specified isVertical get accessor as in:

@property (assign, getter=isEditable) BOOL editable;
@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 16, 2015

Member

Yep, the latter

Member

jwe-apple commented Apr 16, 2015

Yep, the latter

@rsanchezsaez rsanchezsaez changed the title from Vertical scale answer format to Add vertical scale answer format Apr 16, 2015

rsanchezsaez added some commits Apr 17, 2015

ORKScaleSlider: rename 'isVertical' property to 'vertical' with speci…
…fied 'isVertical' getter

(Coding Guidelines for Cocoa)
Replace ORKVerticalScaleAnswerFormat and ORKContinuousVerticalScaleAn…
…swerFormat classes by 'vertical' property in ORKScaleAnswerFormat and ORKContinuousScaleAnswerFormat

Also:
- Rename 'minValue' and 'maxValue' to 'minimumValue' and 'maximumValue' (Coding Guidelines for Cocoa)
- Reorder 'defaultValue' before 'steps' in ORKScaleAnswerFormat initializers (for coherence with ORKContinuousScaleAnswerFormat)
@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 17, 2015

Contributor

I have added commits implementing the requested feedback.

Regarding visual layout, I optimized it for iPhone 6 and up. I moved the range labels to the left side of the scale. I left the valueLabel at the top of the view, as I feel it looks nicer this way. I think that there's enough room for the vertical scale with the new iPhone 6 cell height. If you disagree, let me know and I can move the label to left of the scale.

I think that VoiceOver should work straight away as well (I checked with the Accessibility inspector). I cannot test on an actual device right now, can anybody confirm that it works?

Thanks.

Contributor

rsanchezsaez commented Apr 17, 2015

I have added commits implementing the requested feedback.

Regarding visual layout, I optimized it for iPhone 6 and up. I moved the range labels to the left side of the scale. I left the valueLabel at the top of the view, as I feel it looks nicer this way. I think that there's enough room for the vertical scale with the new iPhone 6 cell height. If you disagree, let me know and I can move the label to left of the scale.

I think that VoiceOver should work straight away as well (I checked with the Accessibility inspector). I cannot test on an actual device right now, can anybody confirm that it works?

Thanks.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 17, 2015

Contributor

Here are some iPhone 6 sized screenshots:

continuous vertical scale


vertical scale #1


vertical scale #2

Contributor

rsanchezsaez commented Apr 17, 2015

Here are some iPhone 6 sized screenshots:

continuous vertical scale


vertical scale #1


vertical scale #2

@avocade

This comment has been minimized.

Show comment
Hide comment
@avocade

avocade Apr 17, 2015

This is such a great read! And even greater to have Apple engineers on Github—never thought I'd see the day, but I'm glad I was wrong to doubt.

avocade commented Apr 17, 2015

This is such a great read! And even greater to have Apple engineers on Github—never thought I'd see the day, but I'm glad I was wrong to doubt.

@YuanZhu-apple

This comment has been minimized.

Show comment
Hide comment
@YuanZhu-apple

YuanZhu-apple Apr 18, 2015

Member

One more thing, CurrentThumbImage should be rotated to let its drop shadow point downward.

Member

YuanZhu-apple commented Apr 18, 2015

One more thing, CurrentThumbImage should be rotated to let its drop shadow point downward.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 18, 2015

Contributor

Excellent catch! Issue solved.

Contributor

rsanchezsaez commented Apr 18, 2015

Excellent catch! Issue solved.

@YuanZhu-apple

This comment has been minimized.

Show comment
Hide comment
@YuanZhu-apple

YuanZhu-apple Apr 20, 2015

Don’t use the underscore character as a prefix for your private methods. Apple reserves this convention.
NamingMethods

Don’t use the underscore character as a prefix for your private methods. Apple reserves this convention.
NamingMethods

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 20, 2015

Owner

Fixed.

Owner

rsanchezsaez replied Apr 20, 2015

Fixed.

@YuanZhu-apple

This comment has been minimized.

Show comment
Hide comment
@YuanZhu-apple

YuanZhu-apple Apr 20, 2015

Does thumbImageSubview's transform require to be update repeatedly? What if set _thumbImageNeedsTransformUpdate to NO once the update is done?

Does thumbImageSubview's transform require to be update repeatedly? What if set _thumbImageNeedsTransformUpdate to NO once the update is done?

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 20, 2015

Owner

You are right, that was the idea. I forgot to reset the flag. Fixed.

Owner

rsanchezsaez replied Apr 20, 2015

You are right, that was the idea. I forgot to reset the flag. Fixed.

@YuanZhu-apple YuanZhu-apple self-assigned this Apr 20, 2015

ORKScaleSlider: reset _thumbImageNeedsTransformUpdate flag
Also: don't use '_' prefix for method names.
Show outdated Hide outdated ResearchKit/Common/ORKAnswerFormat.h
minimumValue:(double)scaleMinimum
defaultValue:(double)defaultValue
maximumFractionDigits:(NSInteger)maximumFractionDigits
vertical:(BOOL)verticalFlag;

This comment has been minimized.

@YuanZhu-apple

YuanZhu-apple Apr 20, 2015

Member

Personally, I prefer vertical over verticalFlag.

@YuanZhu-apple

YuanZhu-apple Apr 20, 2015

Member

Personally, I prefer vertical over verticalFlag.

@YuanZhu-apple

This comment has been minimized.

Show comment
Hide comment
@YuanZhu-apple

YuanZhu-apple Apr 20, 2015

Member

ORKTest App(under Testing directory) also needs to be updated. Nice to add few vertical scale testing steps in scale task.

ios simulator screen shot apr 20 2015 12 41 58 pm

Member

YuanZhu-apple commented Apr 20, 2015

ORKTest App(under Testing directory) also needs to be updated. Nice to add few vertical scale testing steps in scale task.

ios simulator screen shot apr 20 2015 12 41 58 pm

rsanchezsaez added some commits Apr 20, 2015

ORKScaleAnswerFormat & ORKContinuousScaleAnswerFormat: handle vertica…
…l property in NSCoding; add it to serialization tests and testTaskModel

Also fix MainViewController scale answer format calls.
@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Added two vertical scale tests to the scale task in ORKTest.

Also added vertical property NSCoding handling, and added it to the serialization tests. Finally, renamed some remaining verticalFlag arguments to vertical.

Contributor

rsanchezsaez commented Apr 21, 2015

Added two vertical scale tests to the scale task in ORKTest.

Also added vertical property NSCoding handling, and added it to the serialization tests. Finally, renamed some remaining verticalFlag arguments to vertical.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Member

Thanks, will review shortly.

Member

jwe-apple commented Apr 21, 2015

Thanks, will review shortly.

@jwe-apple jwe-apple assigned jwe-apple and unassigned YuanZhu-apple Apr 21, 2015

Show outdated Hide outdated ResearchKit/Common/ORKQuestionStepViewController.m
case ORKQuestionTypeVerticalScale:{
height = [ORKSurveyAnswerCellForVerticalScale suggestedCellHeightForView:tableView];
}
break;

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Rather than adding this question type, I'd prefer to change the logic for ORKQuestionTypeScale to interrogate the impliedAnswerFormat of the question step, cast appropriately, and check the vertical property. It might be best to add a method to questionStep like isFormatVertical, which makes the appropriate check. This would be in line with the above code for ORKQuestionTypeSingle/MultipleChoice.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Rather than adding this question type, I'd prefer to change the logic for ORKQuestionTypeScale to interrogate the impliedAnswerFormat of the question step, cast appropriately, and check the vertical property. It might be best to add a method to questionStep like isFormatVertical, which makes the appropriate check. This would be in line with the above code for ORKQuestionTypeSingle/MultipleChoice.

This comment has been minimized.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Yeah, I never felt good about adding a new ORKQuestionTypeVerticalScale value. I didn't realize ORKQuestionStep had these helper methods. I'll improve it.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Yeah, I never felt good about adding a new ORKQuestionTypeVerticalScale value. I didn't realize ORKQuestionStep had these helper methods. I'll improve it.

Show outdated Hide outdated ResearchKit/Common/ORKAnswerFormat.h
@@ -45,6 +45,9 @@ typedef NS_ENUM(NSInteger, ORKQuestionType) {
/// The scale question type asks participants to place a mark at an appropriate position on a continuous or discrete line.
ORKQuestionTypeScale,
/// The vertical scale question type asks participants to place a mark at an appropriate position on a continuous or discrete vertical line.
ORKQuestionTypeVerticalScale,

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Would prefer not to add this as a separate question type. See further comment below.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Would prefer not to add this as a separate question type. See further comment below.

Show outdated Hide outdated ResearchKit/Common/ORKQuestionStepViewController.m
@@ -563,6 +563,7 @@ - (ORKSurveyAnswerCell *)answerCellForTableView:(UITableView *)tableView
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
typeAndCellMapping = @{@(ORKQuestionTypeScale): [ORKSurveyAnswerCellForScale class],
@(ORKQuestionTypeVerticalScale): [ORKSurveyAnswerCellForVerticalScale class],

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Instead of doing this, you can do a special case correction in the code below this to handle the vertical case.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Instead of doing this, you can do a special case correction in the code below this to handle the vertical case.

Show outdated Hide outdated ResearchKit/Common/ORKScaleSlider.m
CGFloat centerY = [self bounds].size.height / 2.0;
- (void)drawRect:(CGRect)rect {
CGRect trackRect = [self trackRectForBounds:self.bounds];
CGFloat centerY = self.bounds.size.height / 2.0;

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

This is ok, but we do prefer the bounds property calls to be written as method calls to make the fact that they're method calls explicit. If I were going to make a change here, for instance, I'd factor out the call to the bounds method and then re-use the struct for both the following calculations.

@jwe-apple

jwe-apple Apr 21, 2015

Member

This is ok, but we do prefer the bounds property calls to be written as method calls to make the fact that they're method calls explicit. If I were going to make a change here, for instance, I'd factor out the call to the bounds method and then re-use the struct for both the following calculations.

This comment has been minimized.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Ah, I see. I replaced it to a property getter call because of UIView public documentation defining it as a property. I'll change it back.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Ah, I see. I replaced it to a property getter call because of UIView public documentation defining it as a property. I'll change it back.

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

It is a property, but ObjC property accesses are still method calls, and they're still relatively expensive compared to accessing (for example) a member of a struct. In places like this, where the ObjC-2.0 syntax can be confusing because it's mixed with struct member access, we tend to use explicit method call syntax.

@jwe-apple

jwe-apple Apr 21, 2015

Member

It is a property, but ObjC property accesses are still method calls, and they're still relatively expensive compared to accessing (for example) a member of a struct. In places like this, where the ObjC-2.0 syntax can be confusing because it's mixed with struct member access, we tend to use explicit method call syntax.

This comment has been minimized.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

I see, thanks for the clarification.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

I see, thanks for the clarification.

Show outdated Hide outdated ResearchKit/Common/ORKScaleSlider.m
CGRect thumbRect = [self thumbRectForBounds:self.bounds trackRect:trackRect value:self.value];
for (UIView *subview in self.subviews) {
if (CGRectEqualToRect(thumbRect, subview.frame)) {
thumbImageSubview = subview;

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

This is ugly and error-prone. It would be better to have and explicitly set a thumb image asset. I'm willing to accept in the short term, though. Please add a comment to this effect and open an issue for fixing in future.

@jwe-apple

jwe-apple Apr 21, 2015

Member

This is ugly and error-prone. It would be better to have and explicitly set a thumb image asset. I'm willing to accept in the short term, though. Please add a comment to this effect and open an issue for fixing in future.

This comment has been minimized.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

I agree that it's hackish and not very future proof, but in my opinion the usage of the actual thumbRect for identifying the subview makes it quite safe. Doesn't it?

Anyway, yeah, using an explicit thumb would be better, but I didn't wan't to bother you guys for a thumb design. I'll add a comment.

@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

I agree that it's hackish and not very future proof, but in my opinion the usage of the actual thumbRect for identifying the subview makes it quite safe. Doesn't it?

Anyway, yeah, using an explicit thumb would be better, but I didn't wan't to bother you guys for a thumb design. I'll add a comment.

Show outdated Hide outdated ResearchKit/Common/ORKScaleSlider.m
UIView *view = nil;
if (_vertical) {
// In vertical mode, we need to ignore the touch area for the needed extra width
const CGFloat desiredSliderWidth = 36.0;

This comment has been minimized.

@jwe-apple

jwe-apple Apr 21, 2015

Member

Can we widen this a little, perhaps to 44?

@jwe-apple

jwe-apple Apr 21, 2015

Member

Can we widen this a little, perhaps to 44?

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

All feedback issues taken care of.

Contributor

rsanchezsaez commented Apr 21, 2015

All feedback issues taken care of.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Rename isFormatVerticalScale, this is very specific to scale formats.

Rename isFormatVerticalScale, this is very specific to scale formats.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Member

With that one naming change I'll merge this.

Member

jwe-apple commented Apr 21, 2015

With that one naming change I'll merge this.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Done.

Contributor

rsanchezsaez commented Apr 21, 2015

Done.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Member

Thanks, great job on this!

Member

jwe-apple commented Apr 21, 2015

Thanks, great job on this!

@jwe-apple jwe-apple merged commit cdff825 into ResearchKit:master Apr 21, 2015

@rsanchezsaez rsanchezsaez deleted the rsanchezsaez:rsanchezsaez-verticalscale branch Apr 21, 2015

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Thanks for the thoughtful comments all along. 👍

Contributor

rsanchezsaez commented Apr 21, 2015

Thanks for the thoughtful comments all along. 👍

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Hey, I think I made a mistake in ORKQuestionStep. One needs to do:

return (ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat).vertical ||
        ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat).vertical);

(the property name) instead of:

return (ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat).isVertical ||
        ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat).isVertical);

(the getter name).

If you clean the project and recompile, Xcode is not happy. I've fixed it in the latest commit in the corresponding branch. I think you can just do a fast-forward merge to keep GitHub clean, but tell me if you want me to submit a separate pull request.

I spotted it when trying to compile the merged master branch. I'm not sure why Xcode was happily compiling on my separate branch.

Contributor

rsanchezsaez commented Apr 21, 2015

Hey, I think I made a mistake in ORKQuestionStep. One needs to do:

return (ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat).vertical ||
        ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat).vertical);

(the property name) instead of:

return (ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat).isVertical ||
        ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat).isVertical);

(the getter name).

If you clean the project and recompile, Xcode is not happy. I've fixed it in the latest commit in the corresponding branch. I think you can just do a fast-forward merge to keep GitHub clean, but tell me if you want me to submit a separate pull request.

I spotted it when trying to compile the merged master branch. I'm not sure why Xcode was happily compiling on my separate branch.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Oh, I figured why it doesn't compile now. Pull request #48 (fix double evaluation in ORKDynamicCast macro) makes the compiler to see the general id type, rather than the casted type, coming from the ORKDynamicCast() macro. Not sure if PR#48 needs to be re-evaluated or not.

Still, the fix to my code still applies, I think.

Contributor

rsanchezsaez commented Apr 21, 2015

Oh, I figured why it doesn't compile now. Pull request #48 (fix double evaluation in ORKDynamicCast macro) makes the compiler to see the general id type, rather than the casted type, coming from the ORKDynamicCast() macro. Not sure if PR#48 needs to be re-evaluated or not.

Still, the fix to my code still applies, I think.

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Actually, my previous fix does not compile either. Explicitly calling the getter works:

return ([ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat) isVertical] ||
        [ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat) isVertical]);

I've overwritten my fixing commit to reflect this. Although I think we may want to go back to an ORKDynamicCast() implementation that returns a properly casted object (and then I'd prefer to just use the .vertical property syntax).

Sorry about the confusion.

Contributor

rsanchezsaez commented Apr 21, 2015

Actually, my previous fix does not compile either. Explicitly calling the getter works:

return ([ORKDynamicCast([self impliedAnswerFormat], ORKScaleAnswerFormat) isVertical] ||
        [ORKDynamicCast([self impliedAnswerFormat], ORKContinuousScaleAnswerFormat) isVertical]);

I've overwritten my fixing commit to reflect this. Although I think we may want to go back to an ORKDynamicCast() implementation that returns a properly casted object (and then I'd prefer to just use the .vertical property syntax).

Sorry about the confusion.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Member

Don't panic. I think the ORKDynamicCast() just needs extra parens. I'll fix it up.

Member

jwe-apple commented Apr 21, 2015

Don't panic. I think the ORKDynamicCast() just needs extra parens. I'll fix it up.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 21, 2015

Member

Just pushed my fix to master, thanks!

Member

jwe-apple commented Apr 21, 2015

Just pushed my fix to master, thanks!

@rsanchezsaez

This comment has been minimized.

Show comment
Hide comment
@rsanchezsaez

rsanchezsaez Apr 21, 2015

Contributor

Perfect.

Contributor

rsanchezsaez commented Apr 21, 2015

Perfect.

@drekryan

This comment has been minimized.

Show comment
Hide comment
@drekryan

drekryan Apr 21, 2015

@jwe-apple 👍 Thanks for that fix!

drekryan commented Apr 21, 2015

@jwe-apple 👍 Thanks for that fix!

syoung-smallwisdom added a commit to syoung-smallwisdom/ResearchKit that referenced this pull request Jun 9, 2016

Merge pull request #33 from Erin-Mounts/non-nullable-start-end-dates
ORKResult start/end dates should not be nullable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment