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

Settings icons are color inverted based on wrong metrix #41352

Closed
nickvergessen opened this issue Nov 9, 2023 · 8 comments · Fixed by #41855
Closed

Settings icons are color inverted based on wrong metrix #41352

nickvergessen opened this issue Nov 9, 2023 · 8 comments · Fixed by #41855

Comments

@nickvergessen
Copy link
Member

nickvergessen commented Nov 9, 2023

The icons of the app-navigation-* are influenced by the dark/bright theme. However for the active entry this behaviour is wrong and it would always need to use the color of var(--color-primary-element-text) instead of a filter var(--background-invert-if-dark)

Basically we need the opposite of

'--primary-invert-if-bright' => $this->util->invertTextColor($this->primaryColor) ? 'invert(100%)' : 'no',

Bright theme Dark theme
Bildschirmfoto vom 2023-11-09 07-18-08 Bildschirmfoto vom 2023-11-09 07-18-04
Bildschirmfoto vom 2023-11-09 07-18-26 Bildschirmfoto vom 2023-11-09 07-18-30
@szaimen
Copy link
Contributor

szaimen commented Nov 29, 2023

I think the problem is rather that the icons are loaded via url and cannot be inverted thus...

@nickvergessen
Copy link
Member Author

I think the problem is rather that the icons are loaded via url and cannot be inverted thus...

But they are inverted, just using the wrong "foundation" color to decide if it should be inverted. as per above I think we just need to add a new constant which does the invert of the existing variable.

@szaimen
Copy link
Contributor

szaimen commented Nov 29, 2023

There is
image

but it breaks the other icons:

image

@szaimen
Copy link
Contributor

szaimen commented Nov 29, 2023

And even better would be using --color-primary-element-text for the icon but that does not work with icons that are loaded via url...

@nickvergessen
Copy link
Member Author

I'll send the filter patch I mentioned above

@nickvergessen
Copy link
Member Author

Patch at #41855

@nickvergessen
Copy link
Member Author

And even better would be using --color-primary-element-text for the icon but that does not work with icons that are loaded via url...

yes, but it would break the API so not an option atm

@szaimen
Copy link
Contributor

szaimen commented Nov 29, 2023

Thanks Joas! :)

@szaimen szaimen added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Nov 29, 2023
@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants