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 attributedText binding target to UITextView and UITextField #3307

Merged
merged 11 commits into from Nov 26, 2016

Conversation

Projects
None yet
2 participants
@augusteo
Copy link
Contributor

augusteo commented Nov 22, 2016

very simple attributedText binding target for UITextView and UITextField.

@@ -7,6 +7,11 @@ extension Reactive where Base: UITextView {
public var text: BindingTarget<String> {

This comment has been minimized.

@andersio

andersio Nov 22, 2016

Member

Makes me wonder if this (and the signals) should be String?, or should we instead hide the nullability (for UITextField too) and consistently use empty strings in our APIs?

This comment has been minimized.

@augusteo

augusteo Nov 23, 2016

Contributor

UITextView text property is optional, so yes, String? would be closer to the truth, the same with label and textfield. Do you want me to make the change?

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

It is somehow IUO though. But yeah, for consistency's sake, please turn them into String?. Thanks.

@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 22, 2016

❌  /Users/travis/build/ReactiveCocoa/ReactiveCocoa/ReactiveCocoaTests/UIKit/UITextViewSpec.swift:32:62: use of undeclared type 'NoError'
                                let (pipeSignal, observer) = Signal<NSAttributedString?, NoError>.pipe()
                                                                                         ^~~~~~~

The UITextViewSpec needs to import enum Result.NoError.

@augusteo augusteo force-pushed the augusteo:attributedText-uitextview branch from 77d3042 to 2c3a6f8 Nov 23, 2016

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 23, 2016

I've imported Result.NoError

@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 23, 2016

May you also add the signals for NSAttributedString?

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 23, 2016

May you also add the signals for NSAttributedString?

what do you mean by that @andersio

@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 23, 2016

attributedTextValues and continuousAttributedTextValues.

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 23, 2016

Ah got it. will do

expect(latestValue) == textView.text
var latestValue: String?
textView.reactive.textValues.observeValues { text in
latestValue = text

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Please fix the Indentation. 😸

it("should emit user initiated changes to its text value continuously") {
textView.text = "Test"
NotificationCenter.default.post(name: .UITextViewTextDidEndEditing,
object: textView)

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Ditto.

var latestValue: String?
textView.reactive.continuousTextValues.observeValues { text in
latestValue = text
}

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Ditto.

object: textView)
expect(latestValue) == textView.text
NotificationCenter.default.post(name: .UITextViewTextDidChange,
object: textView)

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Ditto.

@@ -30,4 +30,32 @@ extension Reactive where Base: UITextView {
public var continuousTextValues: Signal<String, NoError> {

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Can you turn textValues and continuousTextValues to use String? too?

@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 23, 2016

Thanks! It should be ready after these changes. :shipit:

augusteo added some commits Nov 23, 2016

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 23, 2016

should be all good. thanks for the feedback @andersio

@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 23, 2016

Gonna merge after CI test passes.

return makeBindingTarget { $0.attributedText = $1 }
}

private func attributedTextValues(forName name: NSNotification.Name) -> Signal<String, NoError> {

This comment has been minimized.

@andersio

andersio Nov 23, 2016

Member

Oops. The return type is incorrect.

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 23, 2016

sorry, should be fine now

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 24, 2016

i found some test issue where the textview styling is changing the attributed text styling and fails the test. I'll try to fix it.

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 24, 2016

ok the tests are all passing now.

I had to make some changes.
in UITextViewSpec:
added attributes in the test expectation because without it, the textView would override the styles.

in UITextFieldSpec:
adding the two attribute as per textView doesn't work because the textField added a lot more attributes, failing the test. So I just tested the attributedText.string which is quite static, and still shows that the method will send the correct string when triggered

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 24, 2016

CI server errors, should be unrelated to the actual code change:

2016-11-24 22:44:10.608 xcodebuild[1849:6394] Error Domain=IDETestOperationsObserverErrorDomain Code=5 "Early unexpected exit, operation never finished bootstrapping - no restart will be attempted" UserInfo={NSLocalizedDescription=Early unexpected exit, operation never finished bootstrapping - no restart will be attempted}
** TEST EXECUTE FAILED **

*** Building scheme "Nimble-macOS" in Nimble.xcodeproj
xcodebuild timed out while trying to read ReactiveCocoa.xcworkspace 😭
The command "carthage build --no-skip-current --platform mac" exited with 1.
@andersio

This comment has been minimized.

Copy link
Member

andersio commented Nov 24, 2016

@augusteo Could you try running the tvOS tests locally?

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 25, 2016

Ok turns out tvOS doesn't have Georgia font, that's why its breaking on tvOS. I've just replaced it with systemFont(:) which works on both iOS and tvOS now

@augusteo

This comment has been minimized.

Copy link
Contributor

augusteo commented Nov 26, 2016

yay all green!

@andersio andersio merged commit 0bf7b7e into ReactiveCocoa:master Nov 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@augusteo augusteo deleted the augusteo:attributedText-uitextview branch Nov 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment