Added macOS extensions #102
Conversation
|
Hrm tests are "failing" (passing actually but with a compilation warning) because of an issue in |
|
Thanks @fpillet ! Let us know what we can do to assist. |
|
After investigation, it wasn't This is fixed now. Please merge at your convenience. |
freak4pc
left a comment
freak4pc
left a comment
There was a problem hiding this comment.
Really good change ! I'm all for.
Got some notes I think worth discussing around unifying the code for the different OS-level implementation details
| import RxCocoa | ||
| import ObjectiveC | ||
|
|
||
| public extension Reactive where Base: NSButton { |
There was a problem hiding this comment.
I'm not a huge fan of this repetition... What about using OS-specific button aliases ?
e.g. something like
#if os(iOS)
import UIKit
typealias OSButton = UIButton
#elseif os(macOS)
import Cocoa
import AppKit
typealias OSButton = NSButton
#endif
extension Reactive where Base: OSButton {
}Makes even more sense due to the fact it seems this is already done for tvOS: https://github.com/fpillet/Action/blob/896866a30c879089e740d2a94b29bca62cc9eb53/Sources/Action/UIKitExtensions/UIButton%2BRx.swift#L36
There was a problem hiding this comment.
iOS and tvOS have UIButton in common. macOS NSButton has some code differences -- it only have one target, not multiple, so you'll see the code is not the same
There was a problem hiding this comment.
Yeah for sure, but I still think there is room for condition? The differences, I think, are relatively minor IIRC
There was a problem hiding this comment.
There are plenty other macOS controls that don't exist on iOS and we'll have to add support for (I'm currently using NSStepper and NSPopUpButton a lot and would eventually like to include Action support for them as well). I think it wouldn't make sense to share just one class, it's cleaner to have separate code bases because most of the controls are too diverging
There was a problem hiding this comment.
Alright, I definitely don't have a deep enough understanding of macOS to feel very religious about this :) If it makes sense for you let's go with it. There are some styling issues at the bottom (indent and excess void) that could be worth addressing
| import RxSwift | ||
| import RxCocoa | ||
|
|
||
| public extension Reactive where Base: NSControl { |
There was a problem hiding this comment.
Same here, could be
#if os(iOS)
import UIKit
typealias OSControl = UIControl
#elseif os(macOS)
import Cocoa
import AppKit
typealias OSControl = NSControl
#endif
extension Reactive where Base: OSControl {
}There was a problem hiding this comment.
Somewhat agree on this one, the base control is relatively agnostic
|
|
||
| scheduler.scheduleAt(30) { | ||
| trigger.onNext() | ||
| #if swift(>=3.2) |
There was a problem hiding this comment.
Do we need the separate handling? Wouldn't trigger.onNext(()) work across both ?
also, nit: Indentation looks off here
|
+1 On this PR! 👊👊👊 |
|
One of the |
|
@ashfurrow thanks! will make a new release today. |
4 participants
Added
.rx.actionfor macOSNSControlandNSButton, ported tests to match on platform.