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

Rework theming logic to invert header icon color #4776

Closed
justin-sleep opened this issue May 9, 2017 · 5 comments
Closed

Rework theming logic to invert header icon color #4776

justin-sleep opened this issue May 9, 2017 · 5 comments

Comments

@justin-sleep
Copy link
Member

The current method of determining whether or not to invert the color for header icons (after 68a63ad) is insufficient. Currently, it allows darker icons on a fairly dark background, e.g. my current theme color:

1494362061

One possible solution would be to raise the lightness threshold for inverting the colors. This is what it looks like after I raised the threshold past 55 (my background color's lightness):

1494363045

As what looks 'better' may vary from person to person, another possible solution to this would be to simply allow the user a button in the theming interface to toggle dark/light header icons.

Steps to reproduce

  1. Open theming settings
  2. Change background color to a value straddling the lightness threshold (e.g. #745CBA)

Expected behaviour

The header icon colors should not be dark.

Actual behaviour

The header icon colors invert to their dark variants.

@MariusBluem
Copy link
Member

@juliushaertl

@juliushaertl
Copy link
Member

@justin-sleep Makes sense, but we should make sure that there isn't an issue with another color with this new threshold.

@juliushaertl juliushaertl self-assigned this May 15, 2017
@nickvergessen
Copy link
Member

@juliushaertl could you have a look again.

Seems like OCA.Theming.inverted is set to true but not used by the header menu:

dateien_-jobisch_cloud-_2017-07-31_11 14 57

Notifications icon is black because it uses the value.
https://github.com/nextcloud/notifications/blob/ca1977cf6ee28c835985892ff6f74c6f13630db8/js/app.js#L388

This worked in 11, now broken on 12

@MorrisJobke
Copy link
Member

Fixed in #6344

@jancborchardt
Copy link
Member

Good proposal @justin-sleep! More design feedback and contributions are appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants