Add vertical scale answer format #33

rsanchezsaez · 2015-04-15T10:36:41Z

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.

…ted scale (Scale was being set to the maximumValue when you dragged the touch outside the segmented scale through the view's left bound)

…eSliderView: add vertical mode support

…rticalScaleAnswerFormat classes and factory methods Also: ORKAnswerFormat .h: make spacing coherent; add missing ORKContinuousScaleAnswerFormat initializer comment

…icalScaleQuestionStep examples to scaleQuestionTask

…chezsaez-dev

rsanchezsaez · 2015-04-15T12:52:40Z

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?

YuanZhu-apple · 2015-04-15T21:32:15Z

ResearchKit/Common/ORKAnswerFormat.h

+ORK_CLASS_AVAILABLE
+@interface ORKVerticalScaleAnswerFormat : ORKScaleAnswerFormat
+
+@end


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

YuanZhu-apple · 2015-04-15T22:01:20Z

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:

jwe-apple · 2015-04-15T22:26:07Z

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 · 2015-04-15T22:57:59Z

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.

…chezsaez-verticalscale

jwe-apple · 2015-04-15T23:59:30Z

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 · 2015-04-16T00:56:48Z

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 · 2015-04-16T03:57:18Z

Yep, the latter

…fied 'isVertical' getter (Coding Guidelines for Cocoa)

…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)

…rveyAnswerCellForVerticalScale

rsanchezsaez · 2015-04-17T14:20:51Z

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 · 2015-04-17T17:20:03Z

Here are some iPhone 6 sized screenshots:

avocade · 2015-04-17T23:14:47Z

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 · 2015-04-18T18:32:32Z

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

… scale

rsanchezsaez · 2015-04-18T20:17:30Z

Excellent catch! Issue solved.

jwe-apple · 2015-04-21T17:14:58Z

Thanks, will review shortly.

jwe-apple · 2015-04-21T17:40:57Z

ResearchKit/Common/ORKQuestionStepViewController.m

+        case ORKQuestionTypeVerticalScale:{
+            height = [ORKSurveyAnswerCellForVerticalScale suggestedCellHeightForView:tableView];
+        }
+            break;


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.

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

…cale' type by 'isVertical' method querying

…ead of 'self.bounds'

rsanchezsaez · 2015-04-21T18:35:27Z

All feedback issues taken care of.

jwe-apple · 2015-04-21T18:47:09Z

With that one naming change I'll merge this.

rsanchezsaez · 2015-04-21T19:19:34Z

Done.

jwe-apple · 2015-04-21T19:43:01Z

Thanks, great job on this!

rsanchezsaez · 2015-04-21T19:54:20Z

Thanks for the thoughtful comments all along. 👍

rsanchezsaez · 2015-04-21T20:20:26Z

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 · 2015-04-21T20:33:08Z

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 · 2015-04-21T20:42:48Z

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 · 2015-04-21T20:57:04Z

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

jwe-apple · 2015-04-21T21:35:33Z

Just pushed my fix to master, thanks!

rsanchezsaez · 2015-04-21T21:40:47Z

Perfect.

drekryan · 2015-04-21T21:45:42Z

@jwe-apple 👍 Thanks for that fix!

…-end-dates ORKResult start/end dates should not be nullable

rsanchezsaez added 6 commits April 15, 2015 10:49

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

0b5f1d7

…ted scale (Scale was being set to the maximumValue when you dragged the touch outside the segmented scale through the view's left bound)

ORKScaleSlider: add vertical scale mode

e270c98

ORKScaleAnswerFormatProvider protocol: add isVertical method; ORKScal…

87fd153

…eSliderView: add vertical mode support

ORKAnswerFormat: add ORKVerticalScaleAnswerFormat and ORKContinuousVe…

fd2ab4b

…rticalScaleAnswerFormat classes and factory methods Also: ORKAnswerFormat .h: make spacing coherent; add missing ORKContinuousScaleAnswerFormat initializer comment

TaskListRow: add DiscreteVerticalScaleQuestionStep and ContinuousVert…

5953891

…icalScaleQuestionStep examples to scaleQuestionTask

Merge branch 'master' of github.com:ResearchKit/ResearchKit into rsan…

ddcb2ce

…chezsaez-dev

YuanZhu-apple reviewed Apr 15, 2015
View reviewed changes

Merge branch 'master' of github.com:ResearchKit/ResearchKit into rsan…

00aa321

…chezsaez-verticalscale

brucehappy mentioned this pull request Apr 16, 2015

Scale answer support in form items (issue #4) #41

Merged

rsanchezsaez changed the title ~~Vertical scale answer format~~ Add vertical scale answer format Apr 16, 2015

rsanchezsaez added 4 commits April 17, 2015 12:40

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

99bc310

…fied 'isVertical' getter (Coding Guidelines for Cocoa)

Allow different cell heights in ORKSurveyAnswerCellForScale and ORKSu…

e0e216e

…rveyAnswerCellForVerticalScale

Relayout vertical ORKScaleSliderView (optimized for iPhone 6 and up)

4362e13

rsanchezsaez added 2 commits April 18, 2015 21:13

ORKScaleSlider: keep thumbImage view original orientation in vertical…

c7d27e2

… scale

ORKScaleSlider: improve code formatting (braces, whitespace)

553b277

Add copyright line to ORKScaleSlider & ORKScaleSliderView

62cff57

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

jwe-apple reviewed Apr 21, 2015
View reviewed changes

rsanchezsaez added 2 commits April 21, 2015 19:24

ORKAnswerFormat: & ORKQuestionStep: replace 'ORKQuestionTypeVerticalS…

aed6935

…cale' type by 'isVertical' method querying

ORKScaleSlider: revert to explicit use of '[self bounds]' method inst…

ed88a71

…ead of 'self.bounds'

rsanchezsaez mentioned this pull request Apr 21, 2015

Add custom thumb assets for scales #67

Closed

rsanchezsaez added 2 commits April 21, 2015 19:32

ORKScaleSlider: add comment about thumb rotation issue

3d5abb0

ORKScaleSlider: widen vertical scale tap target

881f01a

ORKQuestionStep: rename 'isFormatVertical' to 'isFormatVerticalScale'

cdff825

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

rsanchezsaez deleted the rsanchezsaez-verticalscale branch April 21, 2015 19:51

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

Merge pull request ResearchKit#33 from Erin-Mounts/non-nullable-start…

b3a689b

…-end-dates ORKResult start/end dates should not be nullable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vertical scale answer format #33

Add vertical scale answer format #33

rsanchezsaez commented Apr 15, 2015

rsanchezsaez commented Apr 15, 2015

YuanZhu-apple Apr 15, 2015

YuanZhu-apple commented Apr 15, 2015

jwe-apple commented Apr 15, 2015

rsanchezsaez commented Apr 15, 2015

jwe-apple commented Apr 15, 2015

rsanchezsaez commented Apr 16, 2015

jwe-apple commented Apr 16, 2015

rsanchezsaez commented Apr 17, 2015

rsanchezsaez commented Apr 17, 2015

avocade commented Apr 17, 2015

YuanZhu-apple commented Apr 18, 2015

rsanchezsaez commented Apr 18, 2015

jwe-apple commented Apr 21, 2015

jwe-apple Apr 21, 2015

rsanchezsaez Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

drekryan commented Apr 21, 2015

Add vertical scale answer format #33

Add vertical scale answer format #33

Conversation

rsanchezsaez commented Apr 15, 2015

rsanchezsaez commented Apr 15, 2015

YuanZhu-apple Apr 15, 2015

Choose a reason for hiding this comment

YuanZhu-apple commented Apr 15, 2015

jwe-apple commented Apr 15, 2015

rsanchezsaez commented Apr 15, 2015

jwe-apple commented Apr 15, 2015

rsanchezsaez commented Apr 16, 2015

jwe-apple commented Apr 16, 2015

rsanchezsaez commented Apr 17, 2015

rsanchezsaez commented Apr 17, 2015

avocade commented Apr 17, 2015

YuanZhu-apple commented Apr 18, 2015

rsanchezsaez commented Apr 18, 2015

jwe-apple commented Apr 21, 2015

jwe-apple Apr 21, 2015

Choose a reason for hiding this comment

rsanchezsaez Apr 21, 2015

Choose a reason for hiding this comment

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

jwe-apple commented Apr 21, 2015

rsanchezsaez commented Apr 21, 2015

drekryan commented Apr 21, 2015