Skip to content
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

Dismiss modally #101

Merged
merged 5 commits into from
Apr 20, 2016
Merged

Conversation

RuiAAPeres
Copy link
Member

@RuiAAPeres RuiAAPeres commented Apr 16, 2016

What's in this PR

This idea originated from a conversation with @dmcrodrigues, where things like popToRootViewController and other ways of manipulating the UI flow, always end with a side effect ( startWith and observe methods family). This PR an attempt to make these situation, more idiomatic and declarative.

API

Currently it's using MutableProperty<(Bool, (Void -> Void)?)?>, the Bool is for the Animated flag and the (Void -> Void)? for the completion closure. I make the whole tuple an optional to address the initial value of a MutableProperty (in this case the initial value is nil).

So I would appreciate some feedback on it. cc @mdiep @iv-mexx

How to use it

Currently one would do the following:

producer.startWithNext { [weak self] _ in
   self?.dismissViewControllerAnimated(true, completion: nil)
}

With this PR:

self.rex_dismissModally <~ producer.map { _ in (true, nil) } // where `true` is for the `Animated` flag and `nil` for the completion closure

Caveat (Help needed)

There also the cases where one wants to observe the dismiss of a UIViewController:

viewController.rex_dismissModally.signal.observeNext { _ in

}

via:

viewController.dismissViewControllerAnimated(true, completion: nil)

I wasn't able to make this work. I tried the following:

let property = associatedProperty(self, key: &dismissModally, initial: initial, setter: setter)

property <~ rac_signalForSelector(#selector(UIViewController.dismissViewControllerAnimated(_:completion:)))
            .rex_toTriggerSignal()
            .map { _ in nil }

return property

This will actually break the test, but it will make the viewController.dismissViewControllerAnimated(true, completion: nil) work.

Edit1: This seems to be fixed on c015b96

@RuiAAPeres RuiAAPeres force-pushed the rp/DismissViewController branch 3 times, most recently from de8e224 to 88becfa Compare April 17, 2016 03:26
@RuiAAPeres
Copy link
Member Author

Adding in @ikesyo for feedback.

@RuiAAPeres
Copy link
Member Author

RuiAAPeres commented Apr 18, 2016

c015b96 fixes the issue! You can now observe both:

viewController.rex_dismissModally <~ SignalProducer(value: (true, nil))

viewController.dismissViewControllerAnimated(true, completion: nil)

@dmcrodrigues
Copy link
Contributor

I like the ideia and seems something that can be quite useful.

However, I would suggest two small changes:

  • changing the name to something like rex_ dismissViewControllerAnimated or rex_dismissAnimated, I think that rex_dismissModally doesn't indicate clearly and easily the operation being performed behind the scenes;
  • naming tuple's parameters to make it more explicit, e.g. SignalProducer(value: (animated: true, completion: nil)).

In case of positive feedback we can adopt this approach to other operations like UINavigationControllers's popViewControllerAnimated and many others.

@RuiAAPeres
Copy link
Member Author

@dmcrodrigues pushed with rex_dismissAnimated naming, since the method belongs to the UIViewController class.

@RuiAAPeres RuiAAPeres force-pushed the rp/DismissViewController branch from c3d6437 to 63bb581 Compare April 18, 2016 14:59
expectation.fulfill()
}

viewController.rex_dismissModally <~ SignalProducer(value: (true, nil))
viewController.rex_dismissAnimated <~ SignalProducer(value: (true, nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can be explicit about the parameters of the tuple (sometimes tests are used as usage reference).

Rui Peres added 2 commits April 18, 2016 17:19
# Conflicts:
#	Rex.xcodeproj/project.pbxproj
#	Source/UIKit/UIViewController.swift
#	Tests/UIKit/UIViewControllerTests.swift
@neilpa neilpa merged commit f55fc30 into RACCommunity:master Apr 20, 2016
@RuiAAPeres RuiAAPeres deleted the rp/DismissViewController branch April 20, 2016 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants