Add closing Browser Tab on iOS #6
Conversation
| @@ -18,8 +18,14 @@ | |||
| #import <SafariServices/SafariServices.h> | |||
|
|
|||
| @interface CBTBrowserTab : CDVPlugin | |||
| { | |||
There was a problem hiding this comment.
move opening bracket to previous line
|
|
||
| - (void) isAvailable:(CDVInvokedUrlCommand*)command; | ||
| - (void) openUrl:(CDVInvokedUrlCommand*)command; | ||
| - (void) close:(CDVInvokedUrlCommand*)command; |
There was a problem hiding this comment.
Add a space before the asterisk, and remove the one after the (void), i.e.:
- (void)close:(CDVInvokedUrlCommand *)command; (can you update the two previous lines as well?).
| @@ -63,4 +62,11 @@ - (void) openUrl:(CDVInvokedUrlCommand*)command { | |||
| callbackId:command.callbackId]; | |||
| } | |||
|
|
|||
| - (void) close:(CDVInvokedUrlCommand*)command { | |||
| if (!_safari) | |||
There was a problem hiding this comment.
please add brackets, i.e. if (!_safari) {…
| @@ -18,8 +18,14 @@ | |||
| #import <SafariServices/SafariServices.h> | |||
|
|
|||
| @interface CBTBrowserTab : CDVPlugin | |||
| { | |||
| @protected | |||
| SFSafariViewController* _safari; | |||
There was a problem hiding this comment.
can we call it _safariViewController so it's clear it's not mobile safari?
| @@ -50,10 +50,9 @@ - (void) openUrl:(CDVInvokedUrlCommand*)command { | |||
| callbackId:command.callbackId]; | |||
| } | |||
|
|
|||
| SFSafariViewController *sfvc = | |||
| [[SFSafariViewController alloc] initWithURL: url]; | |||
| _safari = [[SFSafariViewController alloc] initWithURL: url]; | |||
There was a problem hiding this comment.
remove space before url, i.e. initWithURL:url]
| exports.close = function(opt_error) { | ||
| var doNothing = function() {}; | ||
| var error = (!opt_error) ? doNothing : opt_error; | ||
| exec(doNothing, error, 'BrowserTab', 'close', []); |
There was a problem hiding this comment.
this line should be 2-space indented, not 4
|
@WilliamDenniss Done! Please review again. |
|
I still don't know if this is the right approach, because a close() method of this form cannot work for Android. @WilliamDenniss do we want platform-specific methods in this library? |
|
Thanks for your change, it was merged as part of #9. |
Fixes #4