New colour generator #23638
New colour generator #23638
Conversation
|
By analyzing the blame information on this pull request, we identified @MorrisJobke, @rullzer and @Kondou-ger to be potential reviewers |
skjnldsv
force-pushed
the
new-placeholder-colours-generator
branch
from
50f5d20 to
dd7ab16
Compare
|
Tested and works 👍 |
skjnldsv
force-pushed
the
new-placeholder-colours-generator
branch
from
0a61c25 to
d1cba54
Compare
|
Rebased with a small fix for hash detection. |
skjnldsv
force-pushed
the
new-placeholder-colours-generator
branch
from
b2a8abb to
400262a
Compare
|
@jancborchardt review? |
|
That is a lot of computation for just a simple color 😕 I did not look at the code in to much detail (yet) but I like to make sure that this can be done (easily) in the same way on the mobile and desktop clients as well. Because we want to show the same colors there for placeholders. |
|
The code is longer than the previous one for sure, but not very heavy. Do you want me to do some benchmarking? |
|
|
||
| // Placeholders are square | ||
| height = this.height() || size || 32; |
There was a problem hiding this comment.
height does not seem to be defined. Can you define it before?
|
👍 once #23638 (diff) is fixed |
|
|
||
| // Already a md5 hash? | ||
| if( !hash.match(/^[0-9a-f]{32}$/g) ) { | ||
| var hash = md5(hash); |
There was a problem hiding this comment.
It seems like hash was already defined. The var is not needed here.
|
@skjnldsv no I'm fine but it was just a 'woah' moment ;) |
|
@rullzer I was too curious, so I did it anyway! This shouldn't be a that much of a deal ;) |
skjnldsv
force-pushed
the
new-placeholder-colours-generator
branch
from
66d1226 to
dcc28a3
Compare
|
@LukasReschke Rebased with the requested fixes. |
|
👍 this is good to go now imo |
|
@skjnldsv can you show screenshots of a bunch of generated colors before and after? Like a sample from the user mgmt with avatars? :) |
|
Sure |
|
Cool, definitely more readable and less intense! 👍 @skjnldsv can you then make sure that anywhere we use generated colors (Mail app account colors, Contacts, profile picture placeholders, etc) this new generator is used? |
|
@jancborchardt I will look into it yeah! |
|
@skjnldsv thanks! :) |
|
Mail seems good, core too with users list. I may have missed something tho :) |
|
@jancborchardt We should separate the function that convert the hash into a hsl color from the dom manipulation (height, width, text...) because we might want to use it for other things than an avatar. Like the contact header color for example. |
|
I'm checking it right now. |
|
Activities: ok |
|
Ah yeah, @v1r0x the endpoint for Contacts moved with 9.0, cc @DeepDiver1975 @Henni |
|
Sharing: ok |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
See owncloud/contacts#261 (comment)
@jancborchardt