You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by syndbg <gi...@git.apache.org> on 2015/06/11 10:27:19 UTC
[GitHub] cordova-plugin-inappbrowser pull request: [] Fix zoom option not r...
GitHub user syndbg opened a pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104
[] Fix zoom option not respected on Android.
Also minor whitespaces fixes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/syndbg/cordova-plugin-inappbrowser master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #104
----
commit fe0dcc5f2e7a5c046f5c6a9a3e5e39471b0a4592
Author: aantonov <aa...@asteasolutions.com>
Date: 2015-06-11T08:23:45Z
Fix zoom option not respected on Android.
Also minor whitespaces fixes.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by syndbg <gi...@git.apache.org>.
Github user syndbg commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-112974072
@purplecabbage Good catch. In the JIRA ticket, unfortunately I've pasted the wrong option parameter, `location`, when in fact I meant `zoom`.
This PR fixes `zoom=true` or `zoom=false` not making a difference in the android app.
Can we agree on the fact that fixing (in my opinion) non-formatted code, even in the iOS section of the plugin, should not be a blocker to merge this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by syndbg <gi...@git.apache.org>.
Github user syndbg commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-113228209
Alright. Sorry for the delays guys.
Rebased latest upstream master, didn't commit the iOS file and updated the commit message to make more sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-112232437
Numerous issues here .. removing whitespace from the diff reveals only minor changes.
The changes however do not address the issue as reported in JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by syndbg <gi...@git.apache.org>.
Github user syndbg commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#discussion_r32199198
--- Diff: src/android/InAppBrowser.java ---
@@ -656,7 +647,7 @@ public void onClick(View v) {
WebSettings settings = inAppWebView.getSettings();
settings.setJavaScriptEnabled(true);
settings.setJavaScriptCanOpenWindowsAutomatically(true);
- settings.setBuiltInZoomControls(getShowZoomControls());
+ settings.setBuiltInZoomControls(showZoomControls);
--- End diff --
here's the issue
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-113225945
Agree with @purplecabbage, the iOS whitespace stripping is a blocker.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-113302682
I have tested the change, and verified it. So it is now merged.
Thanks for the contribution @syndbg
Some great next steps would be to document the feature (under android quirks) and to add a manual test to the list in the /tests/tests.js file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-112232035
Why does this pull request touch iOS? Can you make it so that it's only Android?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-112974712
It is a blocker typically. Modifications should be restricted to resolving the issue at hand, or at the bare minimum whitespace/formatting changes should be in their own commit.
Reviewing pull requests is enough work without the overhead of having to manually sort through what is relevant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-9158 - InAppBrowser z...
Posted by syndbg <gi...@git.apache.org>.
Github user syndbg commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/104#issuecomment-112385310
The white spaces are automatically stripped by my text editor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org