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

New color generator algorithm #8535

Merged
merged 4 commits into from Apr 12, 2018
Merged

New color generator algorithm #8535

merged 4 commits into from Apr 12, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Feb 26, 2018

Fix #1747

  • We now generate all of our colours based on the three primary we decided to use.
    • Colors are now based on nextcloud official
    • This removes the unwanted hue system we've been using before
    • We can decide to change the sat/lum of one of the primary we want and the generator will automatically smooth the colors to match.
    • Better contrasts
  • New hash to int algorithm.
    • Far better results. A bit slower, but we get a variance around <1% across the desired number.
      Chart for 1M generated hash with 20 colors spread
      chart

@rullzer do you want me to update the php algorithm you used to generate all avatars?

@skjnldsv skjnldsv added enhancement design Design, UI, UX, etc. 2. developing Work in progress papercut Annoying recurring issue with possibly simple fix. labels Feb 26, 2018
@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Feb 26, 2018
@skjnldsv skjnldsv self-assigned this Feb 26, 2018
@rullzer
Copy link
Member

rullzer commented Feb 26, 2018

sure

@skjnldsv skjnldsv changed the title New algorithm New color generator algorithm Feb 26, 2018
@jancborchardt
Copy link
Member

@skjnldsv before & after screenshots would be appreciated for quicker comparison and easier design review. :)

@skjnldsv
Copy link
Member Author

Of course :)
From #1747 (comment)

capture d ecran_2018-02-26_10-37-28
capture d ecran_2018-02-26_10-47-17

@jancborchardt
Copy link
Member

Hm, those look a bit dark and gloomy. How about we instead simply use the Material design color palette? https://material.io/guidelines/style/color.html#color-color-palette

We could use all of the colors which work with the white text on them (except the 900 values cause they are a bit too dark). What do you think?

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 26, 2018

@jancborchardt the main goal was to use stuff with the nextcloud color.
We can indeed use a palette like material, but if we decide to only use those where white font is ok on this, we can drop all yellow-ish based colours :/

Nonetheless, if you want to try new colours generation, you can play with the values in https://github.com/nextcloud/server/pull/8535/files#diff-f0151c05e04ac2dddb3c73f949683689R100

Just find a red, yellow and blue you like and the generator will complete the missing colors according to your demand :)

Here is a live version so you can try it: https://codepen.io/skjnldsv/pen/PQOwQm

@jancborchardt
Copy link
Member

Hmm ok, in the Codepen it actually looks fine. Then since it’s an improvement over the current way, let’s go for it. :)

@skjnldsv
Copy link
Member Author

Yeah, maybe too much colors in the same window isn't the best way to overview this pr! :/

@MorrisJobke
Copy link
Member

I like it 👍

@codecov
Copy link

codecov bot commented Feb 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@88c1b8e). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #8535   +/-   ##
=========================================
  Coverage          ?   51.33%           
  Complexity        ?    25288           
=========================================
  Files             ?     1537           
  Lines             ?    87127           
  Branches          ?        0           
=========================================
  Hits              ?    44729           
  Misses            ?    42398           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
lib/private/Avatar.php 79.42% <100%> (ø) 45 <8> (?)

@skjnldsv
Copy link
Member Author

capture d ecran_2018-02-27_14-42-40

Since we're using nextcloud color, it could lead to strange behavior. @nextcloud/designers are you ok with that or should we use a border? :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 27, 2018

Suggestion if you don't like without borders @nextcloud/designers :

border-radius: 50%;
box-shadow: 0 0 4px 0px rgba(0, 0, 0, 0.3);

Personally I'm okay with or without, I don't mind.

kazam_screenshot_00001 kazam_screenshot_00004

kazam_screenshot_00005 kazam_screenshot_00006

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 27, 2018
@pixelipo
Copy link
Contributor

pixelipo commented Feb 27, 2018

IMO we should use all but the primary colour. If I'm not mistaken this is never used for the UI elements, so it makes no sense to use UI colour either. Better to use !primary because it needs to stand out from the regular interface (avatars, charts etc.)

In the Contacts app it would also be strange if the contact had a header that is the same colour as the NC header:
image

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 27, 2018
@skjnldsv
Copy link
Member Author

@pixelipo could you propose three colours and post the screenshotted palette here please?
https://codepen.io/skjnldsv/pen/PQOwQm?editors=0010

@pixelipo
Copy link
Contributor

@skjnldsv how about this:
palette
CodePen

It's basically a triad of adjacent colors that are complementary to the "NC blue" - so, in a way, a "perfect opposite" :)

@skjnldsv
Copy link
Member Author

@pixelipo I understand your point but I disagree with your choice. The colours should not stand out of the page that much and we should use all of the existing tones (r g & b). Well, it's my opinion :)

@pixelipo
Copy link
Contributor

@skjnldsv those colours are still complementary, which means they will "look good" with our brand colour. The whole point of thir PR is to paint non-UI elements - content, that is to say - to be quickly recognizible.

By the way, you can try larger angles to "grab" a larger portion of the spectrum, e.g. http://paletton.com/#uid=63t0X0k++IruKZLQU+V+Wtt+an5

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 28, 2018

For reference with pixelipo's colours:
kazam_screenshot_00007
kazam_screenshot_00008

@danxuliu
Copy link
Member

danxuliu commented Mar 2, 2018

Like @pixelipo, I would not use the primary colour as one of the base colours. However, I also agree with @skjnldsv that it would be better to use a broader set of tones, and with @jancborchardt that we should only use colours on which white is legible. Moreover, the colours should not be hardcoded based on the Nextcloud colour, but depend on whichever primary colour is set in the theme.

It may be possible to use triads/tetrads to always get the best base colours based on the primary colour that meets all those requeriments, I do not know (but I am probably asking for too much :-P ) .

Here is an alternative idea (totally untested, probably stupid :-P so feel free to ignore it ;-) ) to fulfill all those conditions:

  • We keep a database of all the Material colours in which white is legible (not only main colours, any tone in the 50-A700 range)
  • The three base colours for the colour generator coded by @skjnldsv are choosen as follows:
    • First colour is the nearest whitelisted Material colour (not only main colours, any tone in the 50-A700 range) to the opposite of the primary colour set in the theme
    • Second colour is a random colour from the whitelisted Material colours with the same tone (50-A700 value) as the first colour which is not similar to the primary colour nor the first colour (I guess that there are some easy mathematical checks with the HSV or something like that to know if a colour is similar to another one, but I do not really know ;-) )
    • Third colour is a random colour from the whitelisted Material colours with the same tone as the first and second colours which is not similar to the primary colour, nor the first colour, nor the second colour

@pixelipo
Copy link
Contributor

pixelipo commented Mar 2, 2018

I have to more variants for consideration:

  1. tetrad (Codepen) is more pink/red
var color1 = new Color(236, 0, 87);
var color2 = new Color(125, 241, 0);
var color3 = new Color(255, 139, 0);
  1. complementary triad (CodePen) is more purple/blue
var color1 = new Color(79, 0, 208);
var color2 = new Color(125, 241, 0);
var color3 = new Color(255, 139, 0);

@juliushaertl
Copy link
Member

  1. tetrad (Codepen) is more pink/red

That looks best to me, we have no conflict with colors that are to similar to the primary color (NC-blue) and have a variety of colors, not just using red-ish ones.

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 7, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 28, 2018

Could we take a position on this? :)
@nextcloud/designers

@rullzer
Copy link
Member

rullzer commented Apr 6, 2018

@nextcloud/designers your input is required

@rullzer
Copy link
Member

rullzer commented Apr 6, 2018

Anyway

tetrad (Codepen) is more pink/red

looks fine to me.

@juliushaertl
Copy link
Member

As already said tetrad (Codepen) is more pink/red looks best to me since we don't have collisions with the nextcloud color there. But no strong opinion there, since the others are also a big improvement to the previous color generation mode in terms of contrast and readability.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 6, 2018

Then, let's go for this :)
Please clean up your avatar cache and heads up to your users list ;)

@jancborchardt
Copy link
Member

Hmm … I get the reasoning @pixelipo, and it does look nice. However I see 2 problems:

  • The original intention of the colors, which is to provide a good visual cue for separation, is not that strong anymore because a significant chunk of color is gone
  • It will break down as soon as someone themes their instance and just be devoid of blue? Or does it adjust according to that?

@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 7, 2018

@jancborchardt no, the current theming color is not taken into consideration for now! :)

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

I like the current set and makes sense 👍 Let's get this in and then iterate on this if it is totally off.

Edit: I meant the color set from this comment here: #8535 (comment)

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

👍 for the first version as well from my side

@skjnldsv
Copy link
Member Author

@jancborchardt

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

I also think the original proposal is good and we can always adjust.

For Contacts which have a picture set, we should use a big&blurred version of that picture as header background anyway. :)

@jancborchardt jancborchardt merged commit 04f47a7 into master Apr 12, 2018
@skjnldsv skjnldsv deleted the new-color-generator branch April 12, 2018 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants