-
Notifications
You must be signed in to change notification settings - Fork 39
Fixing a few errors, adding support for attributedPlaceholder #5
Conversation
…m the notification
- Replacing nil string with @"" - Rejecting UITextFieldTextDidChangeNotification when not this object - Updating _storedText in all cases, not just sometimes - Animating the float label in all cases, not just sometimes
[self toggleFloatLabel:UIFloatLabelAnimationTypeHide]; | ||
assert([notification.name isEqualToString:UITextFieldTextDidChangeNotification]); | ||
|
||
if (notification.object == self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is needed if more than one UIFloatLabelTextField
exists at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check needed? I have a sample app another user has sent me with a TableView that has dozens of UIFloatLabelTextFields. They work just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the code before and after. Before, we weren't rejecting notifications that were generated by objects other than ourselves, now we are.
You can see the effectiveness by putting say NSLog(@"0x%x UITextFieldTextDidChangeNotification", self);
in this function. Without the check, you'll see all of the existing UIFloatLabelTextField
s do something with the notification. With the check, only the UIFloatLabelTextField
which actually had some text change will do something.
It may well have worked in the sample app, but it is definitely incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so you're saying that all UIFloatLabelTextField
objects, even ones that are not being interacted with at a given moment are calling textDidChange:
? If so, you discovered, or reaffirmed your hypothesis by checking the memory addresses of objects that are printed out in the log?
Just want to make sure before I take a deeper dive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct - sorry left a bit out of the NSLog
example above. Have edited the comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I confirmed that this issue exists. Checking your solution now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, the new textDidChange: method I have looks like this:
- (void)textDidChange:(NSNotification *)notification
{
assert([notification.name isEqualToString:UITextFieldTextDidChangeNotification]);
if (notification.object == self) {
if ([self.text length]) {
_storedText = [self text];
if (![_floatLabel alpha]) {
[self toggleFloatLabel:UIFloatLabelAnimationTypeShow];
}
} else {
if ([_floatLabel alpha]) {
[self toggleFloatLabel:UIFloatLabelAnimationTypeHide];
}
}
}
}
I kept my conditionals, but wrapped them around your notification.object
conditional check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this may well introduce more bugs - notice that storedText
is only updated if [self.text length]
> 0
. If self.text
has just changed to @""
, _storedText
will still contain whatever was there before.
My addition of != (_storedText.length > 0)
is an efficiency measure to avoid [self toggleFloatLabel:]
being called every time the text changes. With this addition, it only gets called when self.text
either changes from @""
to @"something"
or from @"something"
to @""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be an issue due to an oversight in the original code, where I didn't reset _storedText = @""
in the else
. I think that would avoid the issue outright. I need to think about it for a bit.
if ([self.text length]) {
_storedText = [self text];
if (![_floatLabel alpha]) {
[self toggleFloatLabel:UIFloatLabelAnimationTypeShow];
}
} else {
if ([_floatLabel alpha]) {
[self toggleFloatLabel:UIFloatLabelAnimationTypeHide];
}
_storedText = @""; // Added this now for the example
}
Hey - thanks for these changes! Will take a look today! |
There's actually another small issue I need to fix first before mulling through this request. Working on that now. |
Fixed that issue. Going to push it now. |
No worries :) |
@@ -15,6 +15,7 @@ @interface UIFloatLabelTextField () | |||
@property (nonatomic, assign) CGFloat xOrigin; | |||
@property (nonatomic, assign) CGFloat horizontalPadding; | |||
@property (nonatomic, assign) CGFloat verticalPadding; | |||
@property NSDictionary *placeholderAttributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that @Property declarations default to (nonatomic, strong), but for the sake of code readability - could you add them in in your fork?
Woooooooow. Actually, according to https://developer.apple.com/support/appstore/ iOS 6 is still 9% of the market, but to be honest that's irrelevant. Plenty of developers still need to support iOS 6, and you're providing a library for developers. Why not provide a graceful fallback where you can? |
I'll look into the issue with the text vertical alignment. It will be because I've added the ability to style the placeholder text, and the placeholder text is now different to the normal text. |
Re: iOS 6: I'm of the camp that we shouldn't support stragglers past a certain point, usually 85%. That mindset led me to creating this lib: http://github.com/ArtSabintsev/Harpy. Supporting older versions passed their prime adds more work and more code to the codebase that overtime will be outdated. Hell, even Facebook's flagship app only supports iOS 7+ at this point: https://itunes.apple.com/us/app/facebook/id284882215?mt=8 |
Re: Vertical alignment - thanks. Good luck! |
Alright, I've reviewed and tested most if not all of the code. I'll add some of the minor changes tomorrow morning and push it. At that point, merge your fork with my master. Afterwards, we can review the code again, and decide what to do re:
Sound good? |
Alright, just pushed a bunch of the changes. Feel free to merge with it and add your attributed changes once more, sans iOS 6 support :) |
…r placeholderFont is set.
Closing this PR as I have already merged a bunch of changes. Feel free to resubmit with the other changes if you want then in here. |
No description provided.