Skip to content

Conversation

@malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Oct 29, 2019

Fixes #10695

Changes color of navigation bar to white on Android 8.1+.

The first two columns are API < 27 (without change), the last two columns are API >= 27:
Untitled
@iamthomasbishop I decided to keep the color white on all the screens for the sake of simplicity - it seems the only screens with grey-only background are the settings screens which aren't used that often anyway.
We can enable drawing content below the navigation bar so scrolling content doesn't get cut off as part of another ticket. I tried to quickly implement it and I encountered several issues. So I think this change would need to be planned as part of a sprint.

To test:

  1. Test the navigation bar color on a device with API < 27 and API >=27.

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@malinajirka malinajirka added [Type] Enhancement [Status] Needs Code Review [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Oct 29, 2019
@malinajirka malinajirka added this to the 13.6 milestone Oct 29, 2019
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 29, 2019

You can test the changes on this Pull Request by downloading the APK here.

@iamthomasbishop
Copy link

@malinajirka Just played with a test build, and this already feels much better. I'd be happy to ship as-is, but there might be refinements to be made with regards to the semi-transparency.

One tiny thing I'm wondering if we should try is maybe putting a dim separator on top of the on the 3-button navigation style. The short gesture navigation bar style works beautifully as you have here but the taller one might benefit from a separator. // cc @SylvesterWilmott @mattmiklic any thoughts/preferences?

@iamthomasbishop
Copy link

Also, I'm not sure what this means for our progress on Dark Theme, but we'll want to make sure it looks nice in that theme as well. // cc @khaykov

@mattmiklic
Copy link
Member

FWIW, I've been considering moving to a white background (instead of gray) in light mode as part of the work I'm doing with @khaykov, so there may not be a need for a separator if we end up doing that.

@khaykov
Copy link
Contributor

khaykov commented Oct 29, 2019

@iamthomasbishop @mattmiklic This should not be an issue with a light mode, but in dark mode, the bottom bar has an "elevated" color, so we will need to match it.
Image from Gyazo

Also, elements like this (comment input area)look a bit funky to me:
Image from Gyazo

This probably goes for light mode too.

Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Pixel 4 XL device and it's looking good. Saw the same grey background on the Achievements page as shown below.
Screenshot_20191029-190159

so yes, in the future would probably be good to make it all consistent but for now this change suits the light theme better.

@malinajirka
Copy link
Contributor Author

I'd be happy to ship as-is, but there might be refinements to be made with regards to the semi-transparency.

As I mentioned in the description, if we decide to make this change, I'd rather plan it as it won't be a matter of a couple of hours. We'll need to define which screens need to behave this way and we'll need to make sure actionable items don't appear bellow the bar.

One tiny thing I'm wondering if we should try is maybe putting a dim separator on top of the on the 3-button navigation style. The short gesture navigation bar style works beautifully as you have here but the taller one might benefit from a separator.

Unless I'm missing a simply solution, I believe we'd need to modify layouts of all the screens in the app. So it might not be worth it.

@khaykov If it makes more sense for you to implement this in the dark-theme branch, please feel free to close this PR or change the target branch.

@SylvesterWilmott
Copy link

One tiny thing I'm wondering if we should try is maybe putting a dim separator on top of the on the 3-button navigation style. The short gesture navigation bar style works beautifully as you have here but the taller one might benefit from a separator.

I don't see this being an issue, to be honest. However, Google does do this in its own apps by putting a line on the system navigation bar itself.

@iamthomasbishop
Copy link

As I mentioned in the description, if we decide to make this change, I'd rather plan it as it won't be a matter of a couple of hours. We'll need to define which screens need to behave this way and we'll need to make sure actionable items don't appear bellow the bar.

@malinajirka Of course, I just mean the above solves the bulk of the issue so if/when we're ready we can move forward.

FWIW, I've been considering moving to a white background (instead of gray) in light mode as part of the work I'm doing with @khaykov, so there may not be a need for a separator if we end up doing that.

@mattmiklic Ah, that's right. With that in mind, this approach should blend in nicely. 👍

However, Google does do this in its own apps by putting a line on the system navigation bar itself.

@SylvesterWilmott I wondered this, and when I checked out a handful of Google's apps, I found there isn't consistency – about half of them do have a separator and half don't, so I didn't know what would be preferred. I think it's fine without the separator as well, so we can roll with this.

@malinajirka
Copy link
Contributor Author

@theck13 @jd-alexander I believe all the discussion are settled => it's ready for review/merge.

  • we won't use separator for the navigation bar
  • making the navigation bar semi transparent will be planned as regular task
  • this change shouldn't collide with what Klym is working on

Thank you all ;)!

@theck13 theck13 merged commit ac915f1 into develop Nov 4, 2019
@theck13 theck13 deleted the issue/10695-nav-bar-color branch November 4, 2019 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Needs Design Review A designer needs to sign off on the implemented design. [Type] Enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Change system navigation bar to match UI

8 participants