Skip to content
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

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

Merged
merged 2 commits into from
Oct 12, 2017

Conversation

mmvsk
Copy link
Contributor

@mmvsk mmvsk commented Apr 21, 2017

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:

  • default: dark content (for light backgrounds)
  • lightcontent: light content (for dark backgrounds)
  • blacktranslucent: light content (for dark backgrounds)
  • blackopaque: light content (for dark backgrounds)

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

  • StatusBar.styleDefault()
  • StatusBar.styleLightContent()
  • StatusBar.styleBlackTranslucent()
  • StatusBar.styleBlackOpaque()

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?

  • Default value (no StatusBarStyle parameter in config.xml) in a Cordova project on a Xperia Z3 Android 6.0.1
  • The four StatusBarStyle values (lower and mixed cases) in config.xml in a Cordova project on a Xperia Z3 Android 6.0.1
  • An invalid value "foo" in config.xml in a Cordova project on a Xperia Z3 Android 6.0.1
  • The four StatusBar.style* methods in Chrome Remote Debugger on a Xperia Z3 Android 6.0.1
  • Travis CI on this branch

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

24 tests run, 0 skipped, 0 failed.

@tobiasviehweger
Copy link

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

@filmaj
Copy link
Contributor

filmaj commented Apr 25, 2017

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

Have either of you signed an ICLA?

@tobiasviehweger
Copy link

@filmaj I did, actually.

@filmaj
Copy link
Contributor

filmaj commented Apr 25, 2017

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

@tobiasviehweger
Copy link

@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
Copy link
Contributor Author

mmvsk commented Apr 25, 2017

@filmaj Yes, I also did 3 days ago.

@mmvsk
Copy link
Contributor Author

mmvsk commented Apr 25, 2017

@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
Copy link
Contributor

filmaj commented Apr 25, 2017

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
Copy link

hollyschinsky commented Apr 25, 2017

@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:

  • iOS 7.0+ apps will always default to a lightcontent setting unless the word default is specifically set for StatusBarStyle value in config.xml or programatically.

  • iOS < 7.0 apps will have the behavior explained in my blog post(with working sample app)

@markv
Copy link

markv commented Sep 7, 2017

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

@infil00p
Copy link
Member

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
Copy link
Member

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

@asfgit asfgit merged commit 3d8cff1 into apache:master Oct 12, 2017
asfgit pushed a commit that referenced this pull request Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants