-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add theming logic for share link icons #6400
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
Conversation
| * @param primaryColor the primary color | ||
| */ | ||
| private getColorForPrimary(int primaryColor) { | ||
| if (Color.BLACK == primaryColor) { |
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.
Is this obeying dark mode?
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.
Not at this very moment, we just do this based on the primary color for edge cases and else use the server's text-color value. See also my other comment.
| } else if (Color.WHITE == primaryColor) { | ||
| return Color.BLACK; | ||
| } else { | ||
| return ThemeUtils.fontColor(context, false); |
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.
Isn't this also a good idea to have this computed on server? font-color-light and font-color dark?
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.
Don't know, but @nickvergessen might be able to shed some light on the font-color calculations. I'd hope that with a fallback-ing background color working for dark/light the text-color of a button doesn't have any contrast issues anymore. So we would rather have to discuss if primary button texts would have to be white/black or if we just use white in both theme variants.
So also pinging @jancborchardt about this. ATM we don't have text-color variant from the server (also with element-color reference)
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.
Fine for me to have this in next PR 👍
d1d29bb to
0a41867
Compare
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
0a41867 to
fd47348
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14836.apk |
|
stable-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
|
master-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14836-Screenshot-master |
Codacy398Lint
SpotBugs (new)
SpotBugs (master)
|

Resolve #6399
As a follow-up for #6332 and #6358 the link icon/background should be themed like the fab/primary-action-buttons now available via the #6332 theming logic.