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 new observables for displayed and dismissed states #4

Merged
merged 1 commit into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@twittemb

twittemb commented Oct 8, 2017

This commit add these new features:

  • comment functions in UIViewController+Rx.swift file
  • we can be triggered when a UIViewController:
    • is displayed for the first time
    • has a displayed state change (showed or hidden)
    • is dismissed
    • has its parents dismissed
@codecov-io

This comment has been minimized.

codecov-io commented Oct 8, 2017

Codecov Report

Merging #4 into swift-4.0 will decrease coverage by 0.44%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##           swift-4.0      #4      +/-   ##
============================================
- Coverage      92.75%   92.3%   -0.45%     
============================================
  Files              2       2              
  Lines             69      78       +9     
  Branches           6       7       +1     
============================================
+ Hits              64      72       +8     
  Misses             1       1              
- Partials           4       5       +1
Impacted Files Coverage Δ
Sources/RxViewController/UIViewController+Rx.swift 88% <88.88%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b32c9ae...b240cfb. Read the comment docs.

@devxoul

This comment has been minimized.

Owner

devxoul commented Oct 8, 2017

Please don't change the indentation :(

@twittemb

This comment has been minimized.

twittemb commented Oct 8, 2017

Hi,

I've just pushed a new version where code indentation has not changed.

@@ -50,5 +50,39 @@ public extension Reactive where Base: UIViewController {
let source = self.methodInvoked(#selector(Base.didReceiveMemoryWarning)).map { _ in }
return ControlEvent(events: source)
}

/// Rx observable, triggered when the ViewController has appeared for the first time
public var firstTimeViewDidAppear: Single<Void> {

This comment has been minimized.

@devxoul

devxoul Oct 9, 2017

Owner

I'm not sure firstTimeViewDidAppear is more useful than just using viewDidAppear.take(1) 🤔

}

/// Rx observable, triggered when the ViewController appearance state changes (true if the View is being displayed, false otherwise)
public var isBeingDisplayed: Observable<Bool> {

This comment has been minimized.

@devxoul

devxoul Oct 9, 2017

Owner

It seems useful. 👍 How do you think of the name isVisible?

}

/// Rx observable, triggered when the ViewController or its parent view is being dismissed
public var selfOrParentIsDismissed: Observable<Void> {

This comment has been minimized.

@devxoul

devxoul Oct 9, 2017

Owner

Can you share a use case of this property? I have no idea about the purpose of it.


/// Rx observable, triggered when the ViewController is being dismissed
public var isDismissed: ControlEvent<Bool> {
let source = self.sentMessage(#selector(Base.dismiss)).map { $0.first as? Bool ?? false }

This comment has been minimized.

@devxoul

devxoul Oct 9, 2017

Owner

Is this sentMessage intended? Because, sentMessage is emitted before the dismiss() is called so it doesn't pair with the property name. How about picking one from below:

Property Method
isDismissing sentMessage
isDismissed methodInvoked
@twittemb

This comment has been minimized.

twittemb commented Oct 9, 2017

Hi. I changed my PR according to your remarks. It is pretty hard to test the difference between the dismissing/dismissed states in a XCTestCase. How would you do that ?

@devxoul

This comment has been minimized.

Owner

devxoul commented Oct 10, 2017

Thanks! After some thoughts, isDismissed can be confused because it looks like that it is executed after the completion closure in dismissViewController(). I think we would better keep only isVisible and isDismissing.

Thibault Wittemberg Thibault Wittemberg
Add new observables for visible and dismissed states
We can now be triggered when a UIViewController:
- has a displayed state change (visible or not)
- is dismissing
@twittemb

This comment has been minimized.

twittemb commented Oct 11, 2017

Hi, I've just pushed a new version. thx.

@devxoul devxoul merged commit d669a78 into devxoul:swift-4.0 Oct 11, 2017

1 check passed

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

This comment has been minimized.

Owner

devxoul commented Oct 11, 2017

Thanks for your contribution! 🎉

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