[ORKTextField] Coherent placeholder handling #445

rsanchezsaez · 2015-09-09T01:57:29Z

This PR makes the handing of the placeholder in ORKTextField more similar to what UITextField does.

It comprises of two changes:

Remove extra spacing between unit number and placeholder text. This makes the space between the unit and the placeholder to be consistent between editing and non editing modes.
Do not hide placeholder when the editing text is empty (same as with UITextField).

Also reuses form item cells in ORKStepViewController so cells are not created twice when calculating their height.

Before:

After:

umerkhan-apple · 2015-09-29T21:36:48Z

Did we want to extend this same functionality to the multiline textfields? For example, check out the Message form item under Mini Form in ORKTest.

rsanchezsaez · 2015-09-30T00:29:30Z

@umerkhan-apple: I'll look into this in the following days.

umerkhan-apple · 2015-10-08T21:36:40Z

@rsanchezsaez So did you get a chance to look at multiline textfields? Do we want to extend this functionality to them, or should we just leave it to single line textfields?

rsanchezsaez · 2015-10-19T22:16:15Z

Hey Umer,

Sorry, I missed the GitHub notification earlier.

I agree that the placeholder on text views (multiline text fields) should behave like the text field one. I'm looking into this.

rsanchezsaez · 2015-10-20T01:22:52Z

@umerkhan-apple: Done, I have implemented the same placeholder behavior for ORKAnswerTextView.

umerkhan-apple · 2015-10-20T17:47:01Z

@rsanchezsaez Awesome, I will test the changes shortly.

umerkhan-apple · 2015-10-20T17:54:28Z

ResearchKit/Common/ORKAnswerTextView.m

    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(updateAppearance)
                                                 name:UIContentSizeCategoryDidChangeNotification
                                               object:nil];
+    [[NSNotificationCenter defaultCenter]  addObserver:self selector:@selector(textViewTextDidChange:)


Move selector argument onto a new line just to stay consistent.

rsanchezsaez · 2015-10-20T17:57:12Z

Adding the placeholder to the ORKAnswerTextView was tricky.

First, I tried using a plain UILabel but getting the positioning to be coherent with the typed in text was not easy.

So I settled to adding a non-interactive UITextView as a child of ORKAnswerTextView. For this to layout correctly on form steps, I needed to both set the frame manually (on layout subviews) and add constraints. I find this very weird because normally you either set the frame or use constraints, but never both. Removing one or the other makes it to stop being laid out correctly. If you have a suggestion for cleanest implementation, I'm all ears.

umerkhan-apple · 2015-10-20T18:09:22Z

ResearchKit/Common/ORKAnswerTextView.m


    [self updateAppearance];
 }

+- (void)layoutSubviews {
+    [super layoutSubviews];
+    // Setting the frame directly causes a layout error on a form step (it looks like an iOS bug, as setting the frame should produce the same effect as settings the bounds and the center)


Typo: settings -> setting

umerkhan-apple · 2015-10-20T18:21:18Z

@rsanchezsaez I agree that this is a bit of a tricky implementation. I will look into finding a cleaner approach if possible.

Currently, the framework is crashing if I change orientation from Portrait to Landscape inside ORKTest running Mini Form task.

Console log

2015-10-20 11:16:55.377 ORKTest[614:106204] -[NSIndexPath setExpectedLayoutWidth:]: unrecognized selector sent to instance 0x175dfda0

Tested on iPhone 5 and iPhone 6S.

rsanchezsaez · 2015-10-21T18:37:58Z

I've fixed the crash, that was an unrelated silly mistake on my end.

Let me know if you come up with an alternate solution for the text view placeholder.

YuanZhu-apple · 2015-10-21T21:55:59Z

Not form your code, I just noticed the textview is misaligned in formStep.
Can you fix it in this PR? Thanks!

YuanZhu-apple · 2015-10-21T22:02:32Z

ResearchKit/Common/ORKTextFieldView.m

@@ -185,7 +184,7 @@ - (void)ork_setSuffix:(NSString *)suffix withColor:(UIColor *)color {
    if (suffix.length == 0) {
        return;
    }
-    _suffixLabel = [self ork_createTextLabelWithTextColor:color ?: [UIColor grayColor]];
+    _suffixLabel = [self ork_createTextLabelWithTextColor:(color ?: [UIColor ork_midGrayTintColor])];


Intention of this change?

This color is sometimes used for the unit suffix label when editing an empty ORKTextFieldView with no placeholder. You can see this if you revert this change, tap on the first text field of the first step on Mini Form, tap on the second field, and then tap on the first field again (the unit suffix label turns darker).

ork_midGrayTintColor is used as the color in all other placeholders and unit suffix instances, so I figured it should be used here too.

But maybe we should also review the actual color-chosing logic, because it's weird that this color only gets used the second time you tap on the field but not the first one.

rsanchezsaez · 2015-10-26T10:34:16Z

I'll later look into the alignment of the text view.

umerkhan-apple · 2015-10-26T18:15:12Z

@rsanchezsaez After internal discussion, we have come to the following conclusion:

When there is a unit specified on the text field, keep the behavior as it is in current RK.
When there is no unit specified (basically, when it is functioning like a basic uitextfield) then show the placeholder until editing begins to stay consistent with iOS's uitextfield (your PR's implementation).

rsanchezsaez · 2015-10-26T19:31:58Z

Ok. But why do that for first the case? Don't you think keeping the placeholder until some text is typed is more coherent and consistent?

umerkhan-apple · 2015-10-26T23:02:01Z

The main reason for that conclusion was that having two placeholders seemed untidy and slightly confusing for the user.

rsanchezsaez · 2015-10-27T19:56:42Z

But, in my opinion, the user perception is that, when editing an empty unit-suffixed text field, the unit label becomes part of the placeholder text and then, when they start typing, the "long placeholder" transforms to show only the unit suffix. I think this worked well after reducing the placeholder <> unit separation (see the before/after screenshots above). Don't you think?

If you remain unconvinced let me know and I'll update it to the behavior you requested. :)

YuanZhu-apple · 2015-10-27T20:45:11Z

@rsanchezsaez I think the original set a more clear distinction between placeholder and unit string, see the margin between them when the textfield is inactive.

And when the textfield become active, showing both of them makes the field a little noisy.

[Original]

[This PR]

…ing extra unneeded padding between the placeholder and the unit

…eholder # Conflicts: # ResearchKit/Common/ORKTextFieldView.m

To reproduce, tap on an empty field with unit label and no placeholder (first form item of first step in ORKTest's Miniform).

…ceholder

…ldView This creates a non-interactive 'UITextView' for displaying the placeholder at the exact same position as the 'ORKAnswerTextView' text. It improves encapsulation, the 'ORKAnswerTextView' class handles the placeholder internally. Also: some minor naming convention improvenets in 'ORKTextFieldView'.

…ld empty without triggering answer validity checking This fixes a regression. It restores the behavior in which you are allowed to edit a different text field in a form if you leave the current text field blank. E.g., on ORKTest's 'Mini Form', tapping on the first cell text field and then tapping on second cell text field without having written anything should not trigger the alert.

rsanchezsaez · 2016-01-04T23:54:10Z

I have updated this PR against the latest master, with the requested behavior:

When there is a suffix unit specified on the text field, keep the behavior as it was.
When there is no suffix unit specified, show the placeholder until the user writes text, to stay consistent with iOS's UITextField.

I also fixed a regression in which the user was being prevented leaving a form text field blank (a validation alert was being shown, you can try in ORKTest's Mini Form by editing the first field and trying to move to the second without typing anything).

Let me know if you want additional changes. I think the animation glitch and the suffix color mismatch fixes are worthwhile improvements as well.

The text frame sometimes animated from left to right (e.g., in ORKTest's MiniForm, tap 4th text field, type any number and tap any other text field). Also remove needless period on latest placeholder text.

rsanchezsaez · 2016-01-05T00:15:09Z

Also, the TextView misalign shown in the above comment screenshot no longer seems to be an issue. Maybe iOS 9.2 fixed it? I don't see any RK code change related to that. Please let me know if this is still a problem.

YuanZhu-apple · 2016-01-05T23:39:32Z

R=ME:) @umerkhan-apple Can you take a look as well?

YuanZhu-apple · 2016-01-14T01:12:01Z

@umerkhan-apple Get a chance to try this?

…chezsaez-HomogeneousORKTextFieldView

rsanchezsaez · 2016-03-16T16:23:21Z

@umerkhan-apple Ping? ;-)

…extFieldView [ORKTextField] Coherent placeholder handling

umerkhan-apple reviewed Oct 20, 2015
View reviewed changes

YuanZhu-apple reviewed Oct 21, 2015
View reviewed changes

rsanchezsaez added 11 commits January 4, 2016 21:55

ORKTextFieldView: remove '_unitWithPlaceholder' ivar which was produc…

40111a1

…ing extra unneeded padding between the placeholder and the unit

ORKTextFieldView: make placeholder behave like 'UITextFieldView' plac…

b865da3

…eholder # Conflicts: # ResearchKit/Common/ORKTextFieldView.m

ORKTextFieldView: whitespace

135e220

ORKTextField: reduce left paddingGuess

d39a72e

ORKTextFieldView: fix unit frame animation glitch

1e34cf6

To reproduce, tap on an empty field with unit label and no placeholder (first form item of first step in ORKTest's Miniform).

ORKTextFieldView: fix active unit color being off when there's no pla…

8ab0525

…ceholder

ORKTextFieldView: conform to naming and whitespacing conventions

1b266c3

ORKAnswerTextFieldView: fix whitespace

4048e34

ORKAnswerTextView: fix comment typo

b704be7

ORKTextField: Hide placeholder when editing a field which has a suffix

44ee48f

rsanchezsaez force-pushed the rsanchezsaez-HomogenousORKTextFieldView branch from 99047fe to 2289abf Compare January 4, 2016 23:32

ORKTextFieldView: proper fix for animation glitch

8cbe836

The text frame sometimes animated from left to right (e.g., in ORKTest's MiniForm, tap 4th text field, type any number and tap any other text field). Also remove needless period on latest placeholder text.

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

407de8e

…chezsaez-HomogeneousORKTextFieldView

rsanchezsaez changed the title ~~Coherent 'ORKTextField' placeholder handling~~ [ORKTextField] Coherent placeholder handling Jan 16, 2016

YuanZhu-apple added a commit that referenced this pull request Mar 16, 2016

Merge pull request #445 from rsanchezsaez/rsanchezsaez-HomogenousORKT…

9f5caad

…extFieldView [ORKTextField] Coherent placeholder handling

YuanZhu-apple merged commit 9f5caad into ResearchKit:master Mar 16, 2016

rsanchezsaez deleted the rsanchezsaez-HomogenousORKTextFieldView branch March 17, 2016 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ORKTextField] Coherent placeholder handling #445

[ORKTextField] Coherent placeholder handling #445

rsanchezsaez commented Sep 9, 2015

umerkhan-apple commented Sep 29, 2015

rsanchezsaez commented Sep 30, 2015

umerkhan-apple commented Oct 8, 2015

rsanchezsaez commented Oct 19, 2015

rsanchezsaez commented Oct 20, 2015

umerkhan-apple commented Oct 20, 2015

umerkhan-apple Oct 20, 2015

rsanchezsaez commented Oct 20, 2015

umerkhan-apple Oct 20, 2015

umerkhan-apple commented Oct 20, 2015

rsanchezsaez commented Oct 21, 2015

YuanZhu-apple commented Oct 21, 2015

YuanZhu-apple Oct 21, 2015

rsanchezsaez Oct 26, 2015

rsanchezsaez commented Oct 26, 2015

umerkhan-apple commented Oct 26, 2015

rsanchezsaez commented Oct 26, 2015

umerkhan-apple commented Oct 26, 2015

rsanchezsaez commented Oct 27, 2015

YuanZhu-apple commented Oct 27, 2015

rsanchezsaez commented Jan 4, 2016

rsanchezsaez commented Jan 5, 2016

YuanZhu-apple commented Jan 5, 2016

YuanZhu-apple commented Jan 14, 2016

rsanchezsaez commented Mar 16, 2016

[ORKTextField] Coherent placeholder handling #445

[ORKTextField] Coherent placeholder handling #445

Conversation

rsanchezsaez commented Sep 9, 2015

umerkhan-apple commented Sep 29, 2015

rsanchezsaez commented Sep 30, 2015

umerkhan-apple commented Oct 8, 2015

rsanchezsaez commented Oct 19, 2015

rsanchezsaez commented Oct 20, 2015

umerkhan-apple commented Oct 20, 2015

umerkhan-apple Oct 20, 2015

Choose a reason for hiding this comment

rsanchezsaez commented Oct 20, 2015

umerkhan-apple Oct 20, 2015

Choose a reason for hiding this comment

umerkhan-apple commented Oct 20, 2015

Console log

rsanchezsaez commented Oct 21, 2015

YuanZhu-apple commented Oct 21, 2015

YuanZhu-apple Oct 21, 2015

Choose a reason for hiding this comment

rsanchezsaez Oct 26, 2015

Choose a reason for hiding this comment

rsanchezsaez commented Oct 26, 2015

umerkhan-apple commented Oct 26, 2015

rsanchezsaez commented Oct 26, 2015

umerkhan-apple commented Oct 26, 2015

rsanchezsaez commented Oct 27, 2015

YuanZhu-apple commented Oct 27, 2015

rsanchezsaez commented Jan 4, 2016

rsanchezsaez commented Jan 5, 2016

YuanZhu-apple commented Jan 5, 2016

YuanZhu-apple commented Jan 14, 2016

rsanchezsaez commented Mar 16, 2016