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

Adjust theming color calculations using sRGB luma #7705

Merged
merged 3 commits into from
Jan 9, 2018

Conversation

juliusknorr
Copy link
Member

The theming color calculation is currently using the lightness of a color to decide if icons should be inverted in the app menu or not. However sRGB Luma provides a way better algorithm for deciding if black or white icons should be used on a colored background. (See https://robots.thoughtbot.com/closer-look-color-lightness for reference)

The following example shows the icon inverting the old way (on the left) and the new way (on the right): Color invert comparison old vs. new

Especially for intense color values like #FFFF00 this will provide far better results.

Fixes #7401

@MorrisJobke
Copy link
Member

Especially for intense color values like #FFFF00 this will provide far better results.

Yes - this is awesome. I also noticed that one and nice that it is finally fixed.

@@ -90,19 +90,44 @@ public function elementColor($color) {
* @return float
*/
public function calculateLuminance($color) {
$rgb = $this->hexToRGB($color);
Copy link
Member

Choose a reason for hiding this comment

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

Easier approach to write this code:

list($red, $green, $blue) = $this->hexToRGB($color);

See https://secure.php.net/manual/en/function.list.php#refsect1-function.list-examples

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 8, 2018
@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

Merging #7705 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7705      +/-   ##
============================================
+ Coverage     51.18%   51.18%   +<.01%     
- Complexity    24948    24950       +2     
============================================
  Files          1605     1605              
  Lines         94923    94927       +4     
  Branches       1376     1376              
============================================
+ Hits          48584    48589       +5     
+ Misses        46339    46338       -1
Impacted Files Coverage Δ Complexity Δ
apps/theming/lib/Util.php 92.94% <100%> (+0.44%) 30 <4> (+2) ⬆️
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
lib/private/Server.php 81.43% <0%> (-0.12%) 134% <0%> (ø)
config/config.sample.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️

@juliusknorr juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 8, 2018
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 8, 2018
@juliusknorr
Copy link
Member Author

Please review @nextcloud/theming

@juliusknorr juliusknorr changed the title WIP: Adjust theming color calculations using sRGB luma Adjust theming color calculations using sRGB luma Jan 8, 2018
@nickvergessen
Copy link
Member

This will invalidate the android app's code again?
Or did we already move this out to capabilities?

@tobiasKaminsky
Copy link
Member

As I understand it is only a refined calculation.
On Android we use color, text color and element color for newest servers. On older servers we calculate text and element color ourselves.

@juliusknorr
Copy link
Member Author

Yes, if clients use the text-color entry in the capabilities, there will be no other change required. I guess it's fine for android then. What about @nextcloud/ios ? Do you the servers text color value or do you do your own calculations.

@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Jan 9, 2018
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me :D

@MorrisJobke MorrisJobke mentioned this pull request Jan 9, 2018
18 tasks
@MorrisJobke MorrisJobke merged commit 8257689 into master Jan 9, 2018
@MorrisJobke MorrisJobke deleted the theming-calc-adjust branch January 9, 2018 22:04
@spaeps
Copy link

spaeps commented Jan 16, 2018

I don't know if it should be better open a new issue/pull but, dealing with this topic, this is my view on choosing the right color for icons and text.

Any algorithm would not fit the best choice for every colorscheme and tastes, so I think a new custom parameter should be added in the theming app, as "Inverted" (or "Invert color" or other).

This way an user could manually choose the right foreground color, expecially when the main color is a borderline one (i.e. when a single step value changes the foreground color).

The new option would directly force the "inverted" variable value, by inverting the calculated one and definitely solve the issue forever.

If you like the idea I could open a new issue/pull, or the solution could be integrated in this pull.
Let me know.

@juliushaertl @MorrisJobke @nickvergessen @tobiasKaminsky

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

Successfully merging this pull request may close these issues.

Theming on light color
7 participants