-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
(ios): allow to set "preferredContentMode" #688
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.
I've commented on the issue, it should be a preference or browser option, not default, because showing a browser user agent is an iPadOS feature, we can just disable it for everybody.
I think preference totally makes sense and using this preference for the main webview, too. Changed this ways. Let me know what you think. |
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.
The actual functionality works fine but I think we want to document it in the README.md
so we don't end up with undocumented functionality.
Maybe a "Preferences" subsection under Installation which indicates the end user were the preference needs to live in config.xml
and an example usage, e.g.
<preference name="PreferredContentMode" value="mobile" />
Also need to indicate that this preference is platform-specific to iOS.
@dpa99c Thanks for the review. Yes we need to document. Since we also would need that preference in the cordova-ios platform my plan was to do the documentation in cordova-docs where the other prefrences are. The PR for cordova-ios is to be done, too. |
I agree with dpa99c, the preference is also going to work on ios and should be documented on ionic-docs for ios, but since it also works in the plugin, should be mentioned in the plugin too |
It makes sense to explain this a bit. I pushed an update for README. |
Tests are failing probably because CI uses Xcode 10 and this is new for iOS13 in Xcode 11. |
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.
Travis is still not happy. Otherwise thank & LGTM
Talking about the Travis failure (https://travis-ci.org/github/apache/cordova-plugin-inappbrowser/jobs/694970579): Did the name change from |
I dont really know where |
I think travis changes should go outside of this PR, probably best to sent a separate PR and once the tests are good in that PR, merge it and rebase this one |
@timbru31 https://github.com/ios-control/ios-sim/blob/7624cc7af7e875a04f2f7dc3d5c78f2979eb9908/src/helpers.js#L88-L92 and Which is used because of |
@NiklasMerz the change in CI configurations might fail because it does not exist in paramedic
https://github.com/apache/cordova-paramedic/tree/master/conf/pr/local |
f5a0ac4
to
b8139b8
Compare
It did fail https://travis-ci.org/github/apache/cordova-plugin-inappbrowser/jobs/694985529. I reverted that. |
b8139b8
to
d9242aa
Compare
Looks like I cannot use the Xcode 11 image with this local configuration. paramedic needs to be fixed first. |
Is this good to merge? Failing CI is a seperate issue being worked on. |
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.
two minor bits, otherwise LGTM and I think we can merge
Co-authored-by: Tim Brust <github@timbrust.de>
Platforms affected
ios
Motivation and Context
Closes #687
Description
WIP to show what issue is talking about. See #687
Testing
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)