Skip to content

Black navigation bar for black theme#2569

Merged
TobiGr merged 3 commits intoTeamNewPipe:devfrom
marcovr:dev
Dec 12, 2019
Merged

Black navigation bar for black theme#2569
TobiGr merged 3 commits intoTeamNewPipe:devfrom
marcovr:dev

Conversation

@marcovr
Copy link
Contributor

@marcovr marcovr commented Aug 27, 2019

As discussed in #1494, this changes the software navigation bar to black with light buttons if all of the following points are fulfilled:

  1. The black theme is enabled (maybe this should apply to the dark theme as well?)
  2. The device supports setting a custom navigation bar color (API Level 21).
  3. The device supports setting the navigation buttons color to dark mode (API Level 27).

Fixes #1494

@theScrabi theScrabi modified the milestone: v0.14.2 Sep 24, 2019
@theScrabi theScrabi added the feature request Issue is related to a feature in the app label Sep 24, 2019
@Redirion
Copy link
Member

I would like to propose some (small) changes. Please use the approach described in the answers to this stackoverflow Question: https://stackoverflow.com/questions/32725616/repeating-style-in-v19-v21

It avoids duplicated code and makes the relevant changes easier to see.

Additionally it would be great to have this done similarly to the DarkTheme, not by adding the same black color, but dark themed color to the navigation bar. See styles for the DarkTheme , I think dark_background_color should be fitting.

@marcovr
Copy link
Contributor Author

marcovr commented Oct 15, 2019

Alright, I'll check that out

@marcovr
Copy link
Contributor Author

marcovr commented Oct 15, 2019

Better?

Just tested it again on the emulator (Android 7 and 9) and on a Samsung device (Android 9), everything seems fine.

This is how the DarkTheme now looks:

@Redirion
Copy link
Member

Redirion commented Oct 15, 2019

why the Base.BlackTheme and Base.DarkTheme though?

why not just add

<style name="Base.V7.DarkTheme" parent="DarkTheme" />

in values/styles.xml

and then

<style name="Base.V27.DarkTheme" parent="Base.V7.DarkTheme">

<style name="DarkTheme" parent="Base.V27.DarkTheme" />

in values-v27/styles.xml?

@marcovr
Copy link
Contributor Author

marcovr commented Oct 15, 2019

I agree that it's a bit convoluted (it took me a moment to understand it) but it does make sense:

  • The DarkTheme contains the actual theme items/values
  • The Base.DarkTheme is a fixed reference for DarkTheme and serves as a pointer to the highest available Base.XX.DarkTheme
  • Base.XX.DarkTheme contains version specific additions and links to the next lower available Base.XX.DarkTheme, until at last the lowest one links to the parent style.

@Redirion
Copy link
Member

okay, then just change the V21 to V27 and its perfect! :)

@marcovr
Copy link
Contributor Author

marcovr commented Oct 15, 2019

Oh, how did I miss that? You're right

@Redirion
Copy link
Member

Thank you!

@TobiGr
This would make a nice little addition to 0.17.4

Redirion
Redirion previously approved these changes Oct 15, 2019
Stypox
Stypox previously approved these changes Oct 15, 2019
@TobiGr
Copy link
Contributor

TobiGr commented Dec 11, 2019

Sorry for the late reply. Can someone please sum up what this PR changes? A before/after screenshot would be lovely :) I tested this with an Android 9 emulator and didn't see any difference.

@marcovr
Copy link
Contributor Author

marcovr commented Dec 11, 2019

See this comment for before/after.
In short: it changes the navigation bar color to dark/black in order to match the theme. This only affects phones which (for some reason) use a light navigation bar per default, which looks terrible together with the black theme.

@TobiGr
Copy link
Contributor

TobiGr commented Dec 11, 2019

Thanks! Can you please rebase the commits?

@marcovr
Copy link
Contributor Author

marcovr commented Dec 11, 2019

Alright, I'm on it

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thanks

@TobiGr TobiGr merged commit 6029e18 into TeamNewPipe:dev Dec 12, 2019
This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request Issue is related to a feature in the app

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set color of Navigation bar in the bottom

5 participants