Skip to content

Conversation

@jaapjanfrans
Copy link

Platforms affected

ios

What does this PR do?

Adds the option on ios to enable or disable the close button in the toolbar. I needed this for an app that should not display a close button but should show the toolbar because of the previous/next navigational buttons.

What testing has been done on this change?

Run automated tests as outlined here for platform ios

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@jaapjanfrans
Copy link
Author

so... can this request be considered by a project maintainer?
If any clarification is needed or if questions arise, please let me know :-)

@janpio
Copy link
Member

janpio commented Nov 22, 2017

My iOS coding skills are too bad to review this myself, but have a look at what I posted at this other PR with the same problem: #246 (comment)

@surajpindoria
Copy link
Member

The code for this looks fine and it works as described. But I am struggling to really find a reason for adding this feature.

  • Why would a developer want to have an inappbrowser window with no close button?
  • As a user of the app, how would you close the inappbrowser?
  • Why would this be an iOS specific feature only?

To me this seems like a very specific use case that doesn't warrant adding it back to the main plugin repo.

@jaapjanfrans
Copy link
Author

Hey thanks for chiming in!

Why would a developer want to have an inappbrowser window with no close button?
Simple, because the inappbrowser should not be closed in the app I'm building
As a user of the app, how would you close the inappbrowser?
The user is not supposed to close it :-) The app is basically wrapping a web app using the inappbrowser. It runs it's own javascriptcontext in the main webview of the app. The main webview handles push notifications, log in functionallity, sharing of content to the application from other apps on the device, the inappbrowser show the web app. The user does al his/her work in the web app.
Why would this be an iOS specific feature only?
On android, we just do not show the toolbar at all so the problem does not exist there. But on ios, we want to retain the forward/backwards control of the toolbar because ios has no backbutton in it's ui. So we want to have a toolbar with just the navigational controls. current situation on ios is that you get both, the nav controls and the close button.

To me this seems like a very specific use case that doesn't warrant adding it back to the main plugin repo.
Of course it's a specific use case, i won't argue with that. But why not offer the option when it's so easy to implement. Why limit ourselves to an 'all or nothing' toolbar :-)

@janpio
Copy link
Member

janpio commented Nov 29, 2017

Not OP, but my idea I got when seeing this:

  • OAuth signup + login flow, with prev / next buttons
  • App catches "Cancel" button or redirection to "success" page
  • (Probably iOS only app, waiting for implementation for Android by someone else)

@surajpindoria
Copy link
Member

@jaapjanfrans,

I had a feeling that this is what you were trying to accomplish by removing the close button. Another question, have you tried to release this app to the store before? Typically Apple will not approve apps that are simply a wrapper for a website. They will also check to see how your app functions in an offline mode, which could be another reason it would get rejected. If you have a chance please check out this detailed post by another Cordova contributor with regards to this situation.

With that said, I still stand by my original opinion that this should not be merged in. I agree with you that this is a trivial change, but it would be encouraging behavior that would likely result in peoples apps being rejected by Apple.

@jaapjanfrans
Copy link
Author

I understand your concern @surajpindoria

I think we're save with our app because of the way we add some functionality that is not available in the web variant, but i share your concern.

I think it's a bit far fetched to say that this PR will lead people to the dark side of empty wrapper apps, but i also think the're more important things in the world than this little feature. We spent more time talking about it now than it takes a dev to add the feature to his own codebase if he has the need for this ;-)

So Let's close this one. Thanks for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants