WIP: Make iOS twitter login default using SFSafariViewController #289
Conversation
huiping192
force-pushed
the
feature/twitter-cancel
branch
4 times, most recently
from
077a170
to
10998d4
Compare
huiping192
force-pushed
the
feature/twitter-cancel
branch
from
10998d4
to
2a0b841
Compare
huiping192
changed the title
huiping192
changed the title
There was a problem hiding this comment.
While this change is trying to make it closer to TwitterKit, it introduces some problems:
- Previous clients with custom
SFSafariViewControllerDelegateimplementations would lose their ability to specify behaviour when the SafariViewController is done. This was previous behaviour and we shouldn't remove that. - Embedding a SFSafariViewController in another view controller just so that the parent view controller can be the concrete type that conforms to the delegate isn't an elegant solution. There's a more detailed comment on this inline.
| */ | ||
|
|
||
| #if os(iOS) | ||
| func authorize(withCallback callbackURL: URL, | ||
| presentingFrom presenting: UIViewController?, | ||
| forceLogin: Bool = false, | ||
| safariDelegate: SFSafariViewControllerDelegate? = nil, |
There was a problem hiding this comment.
This is a breaking change. We can't just rip this out since there are clients using this, unless we move this change to be a major update change. I suggest keeping the original code, but fallback to a default delegate when the user doesn't provide any safari delegate.
| embedViewController(viewController: createWebViewController()) | ||
| } | ||
|
|
||
| func embedViewController(viewController: UIViewController) { |
There was a problem hiding this comment.
Not a fan of this at all. We shouldn't embed the view controller just to conform to SFSafariViewControllerDelegate. I propose having a default concrete object that conforms to the delegate that is owned by the Swifter object (that gets nil'd out when it's done) in the event the client provides a nil delegate. Either that, or the Swifter class itself can just conform to the SFSafariViewControllerDelegate that has a default behaviour of dismissing it on completion.
|
@meteochu Thank you for your review. Sorry I don't notice the comment earlier. I`m making this pr status to wip |
huiping192
changed the title
why
iOS authorize method need SFSafariViewControllerDelegate but if developer used TwitterKit before
sometime get confused.
TwitterKit implementation: https://github.com/twitter-archive/twitter-kit-ios/blob/4a69c9df030e979ecc60a320bda75c3797c5b079/TwitterKit/TwitterKit/Social/Syndication/API/TWTRWebAuthenticationFlow.m#L91
how
I create a internal view controller to implement SFSafariViewControllerDelegate inside the authorize method. And handle the SafariViewController event.