Improve UIAlertController APIs in the SwiftCatalog sample app. #87
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
There was a problem hiding this comment.
This is a great idea and I absolutely love the implementation!
There are few things that you would need to fix before we feel comfortable merging it in.
Most importantly - it looks like this PR is sent against a non-existing branch, please send (or change) it to be a PR against master branch.
| extension UIAlertAction { | ||
|
|
||
| /// Action types most commonly used | ||
| public enum actionType{ |
There was a problem hiding this comment.
Nit: Space after actionType and before {
| case destructive(String) | ||
|
|
||
| /// Custom action with title and style | ||
| case custom(String,UIAlertActionStyle) |
There was a problem hiding this comment.
Nit: Space between String and UIAlertActionStyle
Also, we can use labels here... Do you think it would add the nice verbosity to it?
| - parameter handler: Call Back function | ||
| - returns UIAlertAction Instance | ||
| */ | ||
| public func action(handler:((String) -> Void)? = nil) -> UIAlertAction { |
There was a problem hiding this comment.
Nit: Space after handler:
|
|
||
| /** | ||
| Creates the action instance for UIAlertController | ||
| - parameter handler: Call Back function |
There was a problem hiding this comment.
Nit: Add empty line between description and first parameter.
| actionStyle = style | ||
|
|
||
| default: | ||
| title = "Ok" |
| } | ||
|
|
||
| //Creating UIAlertAction instance | ||
| let action = UIAlertAction(title:title,style:actionStyle, handler: {(nativeAction) in |
There was a problem hiding this comment.
Nit: Since the closure is the last argument, you can use the trailing closure syntax aka
return UIAlertAction(title: title, style: actionStyle) { nativeAction in
if let handler = handle {
handler(title)
}
}| - parameter style: UIAlertControllerStyle enum value | ||
| - parameter handler: Handler block/closure for the clicked option. | ||
|
|
||
| - let alert = UIAlertController.init(title:"Confirm",message:"Do you want to quit sign up and login with other account ?",actions:.custom("Yes,.default),.custom("Cancel",.destructive)){ (buttonTitle) in |
There was a problem hiding this comment.
Nit: Please line break this line or trim the message argument, to constrain into 140 columns of text.
| } | ||
| } | ||
|
|
||
| self.present(alert, animated: true, completion: nil) |
There was a problem hiding this comment.
Not sure if this line is relevant, since it suggests presentation from inside a UIViewController. I suggest removing it.
| self.present(alert, animated: true, completion: nil) | ||
|
|
||
| */ | ||
| convenience init(title: String, message: String ,actions:[UIAlertAction.actionType] = [.ok],style: UIAlertControllerStyle = .alert,handler:((String) -> Swift.Void)? = nil) { |
There was a problem hiding this comment.
Please line break and style as follows:
convenience init(title: String,
message: String,
actions: [UIAlertAction.actionType] = [.ok],
style: UIAlertControllerStyle = .alert,
handler: ((String) -> Swift.Void)? = nil) {| convenience init(title: String, message: String ,actions:[UIAlertAction.actionType] = [.ok],style: UIAlertControllerStyle = .alert,handler:((String) -> Swift.Void)? = nil) { | ||
|
|
||
| //initialize the contoller (self) instance | ||
| self.init(title: title, message: message, preferredStyle:style) |
There was a problem hiding this comment.
Nit: preferredStyle: style
|
And apparently, I can actually change it myself. |
nlutsenko
changed the title
|
Superseded by #92 |
Please check the gist for the updated code MyGist