-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Improve categorical color management #5815
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5815 +/- ##
==========================================
+ Coverage 63.73% 63.83% +0.09%
==========================================
Files 374 381 +7
Lines 23325 23446 +121
Branches 2607 2614 +7
==========================================
+ Hits 14867 14966 +99
- Misses 8445 8467 +22
Partials 13 13
Continue to review full report at Codecov.
|
e40ed82
to
30d4bfe
Compare
3136d7e
to
3e8772f
Compare
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.
This looks really great overall! thanks for improving the color model significantly 🥇
Left a couple of pretty minor comments.
export default class CategoricalColorScale { | ||
constructor(colors, sharedForcedColors) { | ||
this.colors = colors; | ||
this.sharedForcedColors = sharedForcedColors; |
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.
could you add a comment that clarifies the distinction between sharedForcedColors
and forcedColors
?
from reading it seems like
sharedForcedColors
takes precedence overforcedColos
if present andsharedForcedColors
is never modified vsforcedColors
which is
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.
sharedForcedColors is an optional parameter that comes from parent (usually CategoricalColorNamespace) and supersede this.forcedColors. This let us do the namespace-level color override across all scales.
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 added more comments in the code.
@@ -0,0 +1,58 @@ | |||
import { TIME_SHIFT_PATTERN } from '../utils/common'; | |||
|
|||
export function cleanValue(value) { |
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.
could be in it's own file but don't feel strongly
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 agree it should not be here at all but have to track down which component really need this cleaning. can be tricky
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.
This might be mostly nvd3
so let's keep it here for now and I will get rid of it in a follow-up PR.
} | ||
|
||
const namespaces = {}; | ||
const DEFAULT_NAMESPACE = 'GLOBAL'; |
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.
any use in exporting this so you could import it for tests? and/or adding color
somewhere in the value for easier debugging.
superset/assets/spec/javascripts/modules/CategoricalColorScale_spec.js
Outdated
Show resolved
Hide resolved
expect(c1).to.equal(c3); | ||
expect(c2).to.equal(c5); | ||
}); | ||
it('returns different color for consecutive items', () => { |
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.
do you think it's worthwhile to test color value recycling logic if # items > # colors
?
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.
Great idea. Just add another test for this.
|
||
// These registration code eventually should go into per-app configuration | ||
// when we migrate to the plug-in system. | ||
getInstance() |
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.
would it be worth moving it to the appSetup
call in common.js
?
20f145e
to
1cb5d61
Compare
Addressed comments and rename |
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.
nice! this LGTM! 🌈
Thank you. Ready to merge when you are.
Best regards,
Krist
…--
Krist Wongsuphasawat
http://kristw.yellowpigz.com
On Sep 12, 2018, 11:18 -0700, Chris Williams ***@***.***>, wrote:
@williaster approved this pull request.
nice! this LGTM! 🌈
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
* Create new classes for handling categorical colors * verify to pass existing unit tests * separate logic for forcing color and getting color * replace getColorFromScheme with CategoricalColorManager * organize static functions * migrate to new function * Remove ALL_COLOR_SCHEMES * move sequential colors to another file * extract categorical colors to separate file * move airbnb and lyft colors to separate files * fix missing toFunction() * Rewrite to support local and global force items, plus namespacing. * fix references * revert nvd3 * update namespace api * Update the visualizations * update usage with static functions * update unit test * add unit test * rename default namespace * add unit test for color namespace * add unit test for namespace * start unit test for colorschememanager * add unit tests for color scheme manager * check returns for chaining * complete unit test for the new classes * fix color tests * update unit tests * update unit tests * move color scheme registration to common * update unit test * rename sharedForcedColors to parentForcedColors * remove import
* Create new classes for handling categorical colors * verify to pass existing unit tests * separate logic for forcing color and getting color * replace getColorFromScheme with CategoricalColorManager * organize static functions * migrate to new function * Remove ALL_COLOR_SCHEMES * move sequential colors to another file * extract categorical colors to separate file * move airbnb and lyft colors to separate files * fix missing toFunction() * Rewrite to support local and global force items, plus namespacing. * fix references * revert nvd3 * update namespace api * Update the visualizations * update usage with static functions * update unit test * add unit test * rename default namespace * add unit test for color namespace * add unit test for namespace * start unit test for colorschememanager * add unit tests for color scheme manager * check returns for chaining * complete unit test for the new classes * fix color tests * update unit tests * update unit tests * move color scheme registration to common * update unit test * rename sharedForcedColors to parentForcedColors * remove import
Rewrite how categorical color scales and color schemes are managed.
ColorSchemeManager
for managing color schemes, with mechanism for registering schemes.CategoricalColorScale
that can be passed around as an object and support color overriding (forcedColors
).CategoricalColorNamespace
which can contain one or more scales. Any color overriding (forcedColors
) are scoped within the namespace. This is useful for dashboard. @graceguo-supercat - you can also add new function for serializing the namespace and use it for saving dashboard color config.bnbColors
) or namespace-level (i.e. all scales within the namespace).getColorFromScheme
, which can be removed in follow-up PR.More details please read the unit test output below which also serves as documentation.
@graceguo-supercat @williaster
Testing