CB-13392(Android & iOS): Display app version on splashscreen #148
Conversation
|
@johnlejardinnier Pretty cool PR. I've specifically called out some folks to review your PR. |
|
@infil00p ah, now I see you already reviewed an earlier version of this PR. @johnlejardinnier in the future, you can just push a new commit to the PR branch so you don't have to close and re-open a new one. |
|
@macdonst Thanks a lot ! Thank you for the tip, this is my first contribution. |
There was a problem hiding this comment.
Thanks for your PR -- cool stuff, and I hope we see more PRs from you in the future! I've left a few specific points in the code itself, but a few general things also stand out:
-
It'd be great if this were available on all supported platforms. I'm a great believer in consistency, and although iOS/Android account for the majority of use, it'd be nice to see this extended to Browser and UWP at least. (Note that this isn't required...)
-
I'd like to see some of the constants become actual constants (rather than magic values) -- especially default values. I can infer some of those things, but it'd be nicer (I think) if they were actual constants.
-
Docs should be clear about where the version # comes from (I wonder if this would eventually need to become a pref as well?)
src/ios/CDVSplashScreen.m
Outdated
| position = (mainScreenWidth / 2) - padding; | ||
| } | ||
|
|
||
| CGRect rect = CGRectMake(position, mainScreenHeight - 30.0, textSize.width, textSize.height); |
There was a problem hiding this comment.
Curious -- what's this look like on an iPhone X? Does this clear the home screen indicator?
| @@ -60,6 +60,63 @@ - (void)observeValueForKeyPath:(NSString*)keyPath ofObject:(id)object change:(NS | |||
| [self updateImage]; | |||
| } | |||
|
|
|||
| - (UIColor *)colorFromHexString:(NSString *)hexString { | |||
There was a problem hiding this comment.
Does Cordova not export any methods to handle color parsing in preferences? (I don't know -- have't looked). If it does, I'd suggest using those, rather than building your own. If not, a few thoughts:
- Would it make sense to support the case where the user forgets "#"?
- Would it make sense to support the backgroundColor pref case where it uses "0x" instead?
There was a problem hiding this comment.
For the moment i will not change this part because i don't know what is the better solution.
There was a problem hiding this comment.
statusbar plugin uses a similar code for this. It's in the plugin, not provided by Cordova. It also checks that the color is a string (it's not a problem here as it's a preference, so I think we can be sure that it's a string. But also checks the length and the #
There was a problem hiding this comment.
Statusbar handles all of this in js here: https://github.com/apache/cordova-plugin-statusbar/blob/master/www/statusbar.js#L75
including shortcut css, like #345 => #334455
|
@kerrishotts Thank you so much for your review. |
|
I think this is a nice feature, thanks for contributing! |
| In this case you'll have `v1.0` | ||
|
|
||
|
|
||
| ###### Customize app version |
There was a problem hiding this comment.
What font will be used? Maybe add an example screenshot here so one can visualize what it will look like.
There was a problem hiding this comment.
For the screenshot, could i commit picture into the repo ?
Or do i need to upload file on the web ?
There was a problem hiding this comment.
Just can just paste images into comments - they will automatically be uploaded and shown.
|
Very nice feature and a good addition to the splashscreen functionality @johnlejardinnier. |
|
Thanks all for your time and reviews :) |
|
github will let you add an image to the pr discussion thread
@purplecabbage
risingj.com
…On Wed, Jan 24, 2018 at 2:25 PM, Jonathan Corral ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#148 (comment)>
:
> +
+Now just configure your app version into the widget version attribute, like that :
+```xml
+<widget version="1.0" >...</widget>
+```
+
+
+###### Prefix app version
+To set a prefix version add the following preference to `config.xml`:
+```xml
+<preference name="SplashScreenAppVersionPrefix" value="v" />
+```
+In this case you'll have `v1.0`
+
+
+###### Customize app version
For the screenshot, could i commit picture into the repo ?
Or i need to upload file on the web ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#148 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC0NjNl2_wG46un_PkImj9o6EW4p6zSks5tN63fgaJpZM4Rf6lh>
.
|
|
This is how you attach an image to a comment ...
Please remove the file from your pr. |
|
The SplashScreen plugin for iOS has been integrated into the If you believe that this is a still needed feature for iOS, please create a new PR on the As for Android, Android still requires this plugin so this PR will still be needed for Android. I would recommend remove the iOS changes from this PR and rebase the PR with master. |
Platforms affected
Android and iOS
What does this PR do?
This is a feature to display the app version on the splashscreen.
Documentation has been updated up to use this feature.
What testing has been done on this change?
Sorry there are no tests for this feature so i couldn't check items below.
Do no hesitate to join me if you need further informations.
Thanks.
Checklist