apache / cordova-plugin-statusbar

Apache Cordova Status Bar Plugin
https://cordova.apache.org/
Apache License 2.0
617 stars 480 forks source link

CB-11858: (android) Add StatusBarStyle feature support for Android M+ #78

Closed mmvsk closed 6 years ago

mmvsk commented 7 years ago

Platforms affected

Android 6.0+ (API level 23+)

What does this PR do?

This PR add StatusBarStyle feature support for Android 6.0+ (Marshmallow, API level 23+).

The existing "StatusBarStyle" preference is used, and according to the README.md, the possible case-insensitive values are:

This value can also be changed with the existing javascript functions:

The default value is "lightcontent" as specified in the README (thus backward compatible with the previous style).

What testing has been done on this change?

Checklist

cordova-qa commented 7 years ago

Cordova CI Build has completed successfully.

Commit - Link Dashboard - Link

24 tests run, 0 skipped, 0 failed.

tobiasviehweger commented 7 years ago

Just fyi, I also did some things a long time ago in #64. Nothing did happen to it though....

filmaj commented 7 years ago

Two PRs for the same feature, wow! Thanks you two.

Have either of you signed an ICLA?

tobiasviehweger commented 7 years ago

@filmaj I did, actually.

filmaj commented 7 years ago

Cool, so we can fall back on your PR should @urmx be unable to. What about you, @urmx ?

tobiasviehweger commented 7 years ago

@filmaj @urmx We probably should also check about the blacktranslucent one. It seems we implemented that differently, but I din't quite remember why.. Also we have other defaults (default vs. lightcontent). I remember changing that to unify the behaviour across iOS and Android. @urmx Any thoughts?

mmvsk commented 7 years ago

@filmaj Yes, I also did 3 days ago.

mmvsk commented 7 years ago

@tobiasviehweger For the blacktranslucent behavior, I used this plugin README:

Use the blackTranslucent statusbar (light text, for dark backgrounds).

And I also had a look on Apple Developer about blackTranslucent:

Deprecated. Use black and set the isTranslucent property to true instead.

So it's the same as black:

Use a black background with light content.

And now both blackTranslucent and blackOpaque seem both deprecated in UIStatusBarStyle (as you mentioned in your PR).

However, the devgirl blog you used as a reference state the opposite:

The default and blacktranslucent values display black foreground text and icons. The lightcontent and blackopaque values display white foreground text and icons.

So if she's right, we also have to change this plugin README.

And for the default value in Android, I used lightContent (white on black) because it's backward compatible with the current style (white status bar text).

filmaj commented 7 years ago

Excellent, thanks for signing the ICLA!

Ping @hollyschinsky - can you check what @urmx and @tobiasviehweger mention here with respect to the blacktranslucent/blackopaque bits mentioned in your blog post?

hollyschinsky commented 7 years ago

@filmaj @urmx @tobiasviehweger It's a bit confusing with iOS because the status bar styling has changed several times and the blacktranslucent and blackopaque were deprecated in iOS7.

My blog post was correct at the time but since the deprecation, the code in the StatusBar plugin has been updated. If you look at these methods now, those two deprecated styles are only specifically applied when the version is pre 7. Otherwise that code will set it to the UIStatusBarStyleLightContent style - which is light foreground color (white text). This is the recommended setting based on the apple docs for those deprecated values.

Also, make note that the lightcontent style preference is automatically added to the platform config.xml when you add the StatusBar plugin for iOS. So unless the developer specifically sets something else for that it in the root project config.xml, that is going to be what is set by default (not to be confused with the default style setting itself which is the opposite and dark foreground color (black text) for light backgrounds :/).

In summary... when using this plugin for iOS:

markv commented 6 years ago

@filmaj @urmx @tobiasviehweger can we get this PR merged? Thanks

infil00p commented 6 years ago

What happens when we test this on Android versions earlier than Android M? We currently support every Android version higher than API 19, so if we could get some more testing on this, that would be greatly appreciated.

infil00p commented 6 years ago

BTW: I'm going to go with this one since it merges more cleanly. I'm going to close #64