(ios): allow to set "preferredContentMode"#688
Conversation
jcesarmobile
left a comment
There was a problem hiding this comment.
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. |
dpa99c
left a comment
There was a problem hiding this comment.
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. |
timbru31
left a comment
There was a problem hiding this comment.
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. |
timbru31
left a comment
There was a problem hiding this comment.
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))