-
Notifications
You must be signed in to change notification settings - Fork 94
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
Swift conversion #36
Swift conversion #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for converting this. I've started with some cursory feedback. I have not yet tested the implementation.
MBFingerTipWindow.swift
Outdated
import Foundation | ||
import UIKit | ||
|
||
class MBFingerTipView: UIImageView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swift modules almost act as namespaces, so we don't need to prefix classes in Swift, and we are trying to move away from 2 letter prefixes for Obj-C classes because MB can clash with Apple’s MobileBackup framework. Would you mind changing this to
@objc(MBXFingerTipView)
class FingerTipView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need the
@objc(MBXFingerTipView)
-prefix for every class in the swift file? (Sorry I am just a junior developer, but I really appreciate every feedback I can get)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public classes and methods that you want to expose to Objective-C. Which got me thinking, the FingerTipClass
should probably be open to allow subclassing.
MBFingerTipWindow.swift
Outdated
@@ -0,0 +1,268 @@ | |||
// | |||
// MBFingerTipWindow.swift | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we usually remove this autogenerated comment in our Swift libraries.
MBFingerTipWindow.swift
Outdated
var fadingOut: Bool = false | ||
} | ||
|
||
class MBFingerTipOverlayWindow: UIWindow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: MBX prefix for Obj-C only
Changed prefix to 'MBX' and only for Objc classes
This is clearly a bit of effort and a useful exercise, so thanks for that, but I’d suggest that we take a step back: why does this project need to be converted to Swift? Are there existing issues that adopting Swift would address? Is the existing implementation suffering in some way? By all means, though, feel free to publish a Swift fork of this project. 🙇 |
Thanks for contribution. I'm closing that PR in favour of a new implementation based on your PR. |
Objective C files converted to Swift 5.