-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat(color): support analogous colors to prevent color conflict #19325
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19325 +/- ##
==========================================
- Coverage 66.65% 66.59% -0.06%
==========================================
Files 1674 1676 +2
Lines 64603 64714 +111
Branches 6500 6520 +20
==========================================
+ Hits 43058 43099 +41
- Misses 19862 19924 +62
- Partials 1683 1691 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
export function getAnalogousColors(colors: string[], results: number) { | ||
const generatedColors: string[] = []; | ||
const ext = 3; |
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.
I'm curious how we landed on 3 for this?
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.
A comment might be useful for future readers.
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.
return result.slice(ext); | ||
}); | ||
|
||
// [[A, AA, AAA], [B, BB, BBB]] => [A, B, AA, BB, AAA, BBB] |
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.
I appreciate the explanation of this interleaving. This is one of those little bits of code where there are 90 ways to do it. We might be able to squeeze some performance here, but I don't think anyone's palette will be big enough to cause a perceivable performance problem anway.
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.
LGTM! Seems pretty clean... left one question on the thread about the 3
value that has me a little confused. I'll also spin up an environment to test it further for anyone interested, e.g. @jinghua-qa or @geido :)
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.221.229.151:8080. Credentials are |
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.
LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
🏷️ preset:2022.13 |
…he#19325) * feat(color): support analogous colors * fix test * fix range * add some comment
SUMMARY
Currently we have a series of color schemes (aribnb, d3, superset...) Since we use recycle colors when number of items exceed available colors, the color conflict is easy to occur. For this case we use
tinycolor.analogous
API to get analogous colors to prevent color conflict and the color generation algorithm comes from color consistency.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before
after
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
cc @rusackas @geido