-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(theming): fix incorrectly inverted favicons #44119
Conversation
Signed-off-by: Simon L <[email protected]>
3087466
to
03d19f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works but not sure if this is intended, because the background color is not the primary color.
The reason the default looks odd is that for our default color we use the accessible color instead (there is a if color === ... then instead use other color
).
If that is the case, then should we change the colors? I like the change visually, but if it reverts accessibility, should it not be kept? |
No in this case it is only the icon. But until we (maybe) get #42977 in we use a hack in DefaultTheme and in Capabilities that changes the primary color if it is Nextcloud blue to some slighly darker blue -> results then in white text color. |
I see! I like both solutions, but it seems that this would be more of a bandaid fix, so we might as well wait for the above PR to (hopefully?) be merged, seems much cleaner :) LGTM for now then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing that! :)
Reason: invertTextColor has changed and is not now incorrectly inverting icons even with the default primary color. This now switches to isBrightColor which more correctly returns what we need here.