-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
3c1210c
Create new classes for handling categorical colors
kristw 385a21a
verify to pass existing unit tests
kristw 1502fcb
separate logic for forcing color and getting color
kristw 1fc211d
replace getColorFromScheme with CategoricalColorManager
kristw e3ddbd5
organize static functions
kristw dd60dc7
migrate to new function
kristw 211a777
Remove ALL_COLOR_SCHEMES
kristw c1aeeb5
move sequential colors to another file
kristw 56f9ef3
extract categorical colors to separate file
kristw f3671c8
move airbnb and lyft colors to separate files
kristw 9894368
fix missing toFunction()
kristw 943ecac
Rewrite to support local and global force items, plus namespacing.
kristw 3300080
fix references
kristw 2efac2e
revert nvd3
kristw 1515f05
update namespace api
kristw cd48bd6
Update the visualizations
kristw 2915afc
update usage with static functions
kristw d60d4ee
update unit test
kristw f456065
add unit test
kristw 75318f4
rename default namespace
kristw 9d457a0
add unit test for color namespace
kristw 00d8090
add unit test for namespace
kristw 8a91c4d
start unit test for colorschememanager
kristw 47c5f20
add unit tests for color scheme manager
kristw e2dba45
check returns for chaining
kristw 500f441
complete unit test for the new classes
kristw 1312dd5
fix color tests
kristw 9dbccd6
update unit tests
kristw d08be39
update unit tests
kristw 06970de
move color scheme registration to common
kristw 527e244
update unit test
kristw 1cb5d61
rename sharedForcedColors to parentForcedColors
kristw 1cf4695
remove import
kristw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
130 changes: 130 additions & 0 deletions
130
superset/assets/spec/javascripts/modules/CategoricalColorNameSpace_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
import { it, describe, before } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import CategoricalColorNamespace, { | ||
getNamespace, | ||
getScale, | ||
getColor, | ||
DEFAULT_NAMESPACE, | ||
} from '../../../src/modules/CategoricalColorNamespace'; | ||
import { registerScheme } from '../../../src/modules/ColorSchemeManager'; | ||
|
||
describe('CategoricalColorNamespace', () => { | ||
before(() => { | ||
registerScheme('testColors', ['red', 'green', 'blue']); | ||
registerScheme('testColors2', ['red', 'green', 'blue']); | ||
}); | ||
it('The class constructor cannot be accessed directly', () => { | ||
expect(CategoricalColorNamespace).to.not.be.a('Function'); | ||
}); | ||
describe('static getNamespace()', () => { | ||
it('returns default namespace if name is not specified', () => { | ||
const namespace = getNamespace(); | ||
expect(namespace !== undefined).to.equal(true); | ||
expect(namespace.name).to.equal(DEFAULT_NAMESPACE); | ||
}); | ||
it('returns namespace with specified name', () => { | ||
const namespace = getNamespace('myNamespace'); | ||
expect(namespace !== undefined).to.equal(true); | ||
expect(namespace.name).to.equal('myNamespace'); | ||
}); | ||
it('returns existing instance if the name already exists', () => { | ||
const ns1 = getNamespace('myNamespace'); | ||
const ns2 = getNamespace('myNamespace'); | ||
expect(ns1).to.equal(ns2); | ||
const ns3 = getNamespace(); | ||
const ns4 = getNamespace(); | ||
expect(ns3).to.equal(ns4); | ||
}); | ||
}); | ||
describe('.getScale()', () => { | ||
it('returns a CategoricalColorScale from given scheme name', () => { | ||
const namespace = getNamespace('test-get-scale1'); | ||
const scale = namespace.getScale('testColors'); | ||
expect(scale).to.not.equal(undefined); | ||
expect(scale.getColor('dog')).to.not.equal(undefined); | ||
}); | ||
it('returns same scale if the scale with that name already exists in this namespace', () => { | ||
const namespace = getNamespace('test-get-scale2'); | ||
const scale1 = namespace.getScale('testColors'); | ||
const scale2 = namespace.getScale('testColors2'); | ||
const scale3 = namespace.getScale('testColors2'); | ||
const scale4 = namespace.getScale('testColors'); | ||
expect(scale1).to.equal(scale4); | ||
expect(scale2).to.equal(scale3); | ||
}); | ||
}); | ||
describe('.setColor()', () => { | ||
it('overwrites color for all CategoricalColorScales in this namespace', () => { | ||
const namespace = getNamespace('test-set-scale1'); | ||
namespace.setColor('dog', 'black'); | ||
const scale = namespace.getScale('testColors'); | ||
expect(scale.getColor('dog')).to.equal('black'); | ||
expect(scale.getColor('boy')).to.not.equal('black'); | ||
}); | ||
it('can override forcedColors in each scale', () => { | ||
const namespace = getNamespace('test-set-scale2'); | ||
namespace.setColor('dog', 'black'); | ||
const scale = namespace.getScale('testColors'); | ||
scale.setColor('dog', 'pink'); | ||
expect(scale.getColor('dog')).to.equal('black'); | ||
expect(scale.getColor('boy')).to.not.equal('black'); | ||
}); | ||
it('does not affect scales in other namespaces', () => { | ||
const ns1 = getNamespace('test-set-scale3.1'); | ||
ns1.setColor('dog', 'black'); | ||
const scale1 = ns1.getScale('testColors'); | ||
const ns2 = getNamespace('test-set-scale3.2'); | ||
const scale2 = ns2.getScale('testColors'); | ||
expect(scale1.getColor('dog')).to.equal('black'); | ||
expect(scale2.getColor('dog')).to.not.equal('black'); | ||
}); | ||
it('returns the namespace instance', () => { | ||
const ns1 = getNamespace('test-set-scale3.1'); | ||
const ns2 = ns1.setColor('dog', 'black'); | ||
expect(ns1).to.equal(ns2); | ||
}); | ||
}); | ||
describe('static getScale()', () => { | ||
it('getScale() returns a CategoricalColorScale with default scheme in default namespace', () => { | ||
const scale = getScale(); | ||
expect(scale).to.not.equal(undefined); | ||
const scale2 = getNamespace().getScale(); | ||
expect(scale).to.equal(scale2); | ||
}); | ||
it('getScale(scheme) returns a CategoricalColorScale with specified scheme in default namespace', () => { | ||
const scale = getScale('testColors'); | ||
expect(scale).to.not.equal(undefined); | ||
const scale2 = getNamespace().getScale('testColors'); | ||
expect(scale).to.equal(scale2); | ||
}); | ||
it('getScale(scheme, namespace) returns a CategoricalColorScale with specified scheme in specified namespace', () => { | ||
const scale = getScale('testColors', 'test-getScale'); | ||
expect(scale).to.not.equal(undefined); | ||
const scale2 = getNamespace('test-getScale').getScale('testColors'); | ||
expect(scale).to.equal(scale2); | ||
}); | ||
}); | ||
describe('static getColor()', () => { | ||
it('getColor(value) returns a color from default scheme in default namespace', () => { | ||
const value = 'dog'; | ||
const color = getColor(value); | ||
const color2 = getNamespace().getScale().getColor(value); | ||
expect(color).to.equal(color2); | ||
}); | ||
it('getColor(value, scheme) returns a color from specified scheme in default namespace', () => { | ||
const value = 'dog'; | ||
const scheme = 'testColors'; | ||
const color = getColor(value, scheme); | ||
const color2 = getNamespace().getScale(scheme).getColor(value); | ||
expect(color).to.equal(color2); | ||
}); | ||
it('getColor(value, scheme, namespace) returns a color from specified scheme in specified namespace', () => { | ||
const value = 'dog'; | ||
const scheme = 'testColors'; | ||
const namespace = 'test-getColor'; | ||
const color = getColor(value, scheme, namespace); | ||
const color2 = getNamespace(namespace).getScale(scheme).getColor(value); | ||
expect(color).to.equal(color2); | ||
}); | ||
}); | ||
}); |
96 changes: 96 additions & 0 deletions
96
superset/assets/spec/javascripts/modules/CategoricalColorScale_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
import { it, describe } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import CategoricalColorScale from '../../../src/modules/CategoricalColorScale'; | ||
|
||
describe('CategoricalColorScale', () => { | ||
it('exists', () => { | ||
expect(CategoricalColorScale !== undefined).to.equal(true); | ||
}); | ||
|
||
describe('new CategoricalColorScale(colors, parentForcedColors)', () => { | ||
it('can create new scale when parentForcedColors is not given', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
expect(scale).to.be.instanceOf(CategoricalColorScale); | ||
}); | ||
it('can create new scale when parentForcedColors is given', () => { | ||
const parentForcedColors = {}; | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green'], parentForcedColors); | ||
expect(scale).to.be.instanceOf(CategoricalColorScale); | ||
expect(scale.parentForcedColors).to.equal(parentForcedColors); | ||
}); | ||
}); | ||
describe('.getColor(value)', () => { | ||
it('returns same color for same value', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
const c1 = scale.getColor('pig'); | ||
const c2 = scale.getColor('horse'); | ||
const c3 = scale.getColor('pig'); | ||
scale.getColor('cow'); | ||
const c5 = scale.getColor('horse'); | ||
|
||
expect(c1).to.equal(c3); | ||
expect(c2).to.equal(c5); | ||
}); | ||
it('returns different color for consecutive items', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
const c1 = scale.getColor('pig'); | ||
const c2 = scale.getColor('horse'); | ||
const c3 = scale.getColor('cat'); | ||
|
||
expect(c1).to.not.equal(c2); | ||
expect(c2).to.not.equal(c3); | ||
expect(c3).to.not.equal(c1); | ||
}); | ||
it('recycles colors when number of items exceed available colors', () => { | ||
const colorSet = {}; | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
const colors = [ | ||
scale.getColor('pig'), | ||
scale.getColor('horse'), | ||
scale.getColor('cat'), | ||
scale.getColor('cow'), | ||
scale.getColor('donkey'), | ||
scale.getColor('goat'), | ||
]; | ||
colors.forEach((color) => { | ||
if (colorSet[color]) { | ||
colorSet[color]++; | ||
} else { | ||
colorSet[color] = 1; | ||
} | ||
}); | ||
expect(Object.keys(colorSet).length).to.equal(3); | ||
['blue', 'red', 'green'].forEach((color) => { | ||
expect(colorSet[color]).to.equal(2); | ||
}); | ||
}); | ||
}); | ||
describe('.setColor(value, forcedColor)', () => { | ||
it('overrides default color', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
scale.setColor('pig', 'pink'); | ||
expect(scale.getColor('pig')).to.equal('pink'); | ||
}); | ||
it('does not override parentForcedColors', () => { | ||
const scale1 = new CategoricalColorScale(['blue', 'red', 'green']); | ||
scale1.setColor('pig', 'black'); | ||
const scale2 = new CategoricalColorScale(['blue', 'red', 'green'], scale1.forcedColors); | ||
scale2.setColor('pig', 'pink'); | ||
expect(scale1.getColor('pig')).to.equal('black'); | ||
expect(scale2.getColor('pig')).to.equal('black'); | ||
}); | ||
it('returns the scale', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
const output = scale.setColor('pig', 'pink'); | ||
expect(scale).to.equal(output); | ||
}); | ||
}); | ||
describe('.toFunction()', () => { | ||
it('returns a function that wraps getColor', () => { | ||
const scale = new CategoricalColorScale(['blue', 'red', 'green']); | ||
const colorFn = scale.toFunction(); | ||
expect(scale.getColor('pig')).to.equal(colorFn('pig')); | ||
expect(scale.getColor('cat')).to.equal(colorFn('cat')); | ||
}); | ||
}); | ||
}); |
141 changes: 141 additions & 0 deletions
141
superset/assets/spec/javascripts/modules/ColorSchemeManager_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
import { it, describe, beforeEach } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import ColorSchemeManager, { | ||
getInstance, | ||
getScheme, | ||
getAllSchemes, | ||
getDefaultSchemeName, | ||
setDefaultSchemeName, | ||
registerScheme, | ||
registerMultipleSchemes, | ||
} from '../../../src/modules/ColorSchemeManager'; | ||
|
||
describe('ColorSchemeManager', () => { | ||
beforeEach(() => { | ||
const m = getInstance(); | ||
m.clearScheme(); | ||
m.registerScheme('test', ['red', 'green', 'blue']); | ||
m.registerScheme('test2', ['orange', 'yellow', 'pink']); | ||
m.setDefaultSchemeName('test'); | ||
}); | ||
it('The class constructor cannot be accessed directly', () => { | ||
expect(ColorSchemeManager).to.not.be.a('Function'); | ||
}); | ||
describe('static getInstance()', () => { | ||
it('returns a singleton instance', () => { | ||
const m1 = getInstance(); | ||
const m2 = getInstance(); | ||
expect(m1).to.not.equal(undefined); | ||
expect(m1).to.equal(m2); | ||
}); | ||
}); | ||
describe('.getScheme()', () => { | ||
it('.getScheme() returns default color scheme', () => { | ||
const scheme = getInstance().getScheme(); | ||
expect(scheme).to.deep.equal(['red', 'green', 'blue']); | ||
}); | ||
it('.getScheme(name) returns color scheme with specified name', () => { | ||
const scheme = getInstance().getScheme('test2'); | ||
expect(scheme).to.deep.equal(['orange', 'yellow', 'pink']); | ||
}); | ||
}); | ||
describe('.getAllSchemes()', () => { | ||
it('returns all registered schemes', () => { | ||
const schemes = getInstance().getAllSchemes(); | ||
expect(schemes).to.deep.equal({ | ||
test: ['red', 'green', 'blue'], | ||
test2: ['orange', 'yellow', 'pink'], | ||
}); | ||
}); | ||
}); | ||
describe('.getDefaultSchemeName()', () => { | ||
it('returns default scheme name', () => { | ||
const name = getInstance().getDefaultSchemeName(); | ||
expect(name).to.equal('test'); | ||
}); | ||
}); | ||
describe('.setDefaultSchemeName()', () => { | ||
it('set default scheme name', () => { | ||
getInstance().setDefaultSchemeName('test2'); | ||
const name = getInstance().getDefaultSchemeName(); | ||
expect(name).to.equal('test2'); | ||
getInstance().setDefaultSchemeName('test'); | ||
}); | ||
it('returns the ColorSchemeManager instance', () => { | ||
const instance = getInstance().setDefaultSchemeName('test'); | ||
expect(instance).to.equal(getInstance()); | ||
}); | ||
}); | ||
describe('.registerScheme(name, colors)', () => { | ||
it('sets schemename and color', () => { | ||
getInstance().registerScheme('test3', ['cyan', 'magenta']); | ||
const scheme = getInstance().getScheme('test3'); | ||
expect(scheme).to.deep.equal(['cyan', 'magenta']); | ||
}); | ||
it('returns the ColorSchemeManager instance', () => { | ||
const instance = getInstance().registerScheme('test3', ['cyan', 'magenta']); | ||
expect(instance).to.equal(getInstance()); | ||
}); | ||
}); | ||
describe('.registerMultipleSchemes(object)', () => { | ||
it('sets multiple schemes at once', () => { | ||
getInstance().registerMultipleSchemes({ | ||
test4: ['cyan', 'magenta'], | ||
test5: ['brown', 'purple'], | ||
}); | ||
const scheme1 = getInstance().getScheme('test4'); | ||
expect(scheme1).to.deep.equal(['cyan', 'magenta']); | ||
const scheme2 = getInstance().getScheme('test5'); | ||
expect(scheme2).to.deep.equal(['brown', 'purple']); | ||
}); | ||
it('returns the ColorSchemeManager instance', () => { | ||
const instance = getInstance().registerMultipleSchemes({ | ||
test4: ['cyan', 'magenta'], | ||
test5: ['brown', 'purple'], | ||
}); | ||
expect(instance).to.equal(getInstance()); | ||
}); | ||
}); | ||
describe('static getScheme()', () => { | ||
it('is equivalent to getInstance().getScheme()', () => { | ||
expect(getInstance().getScheme()).to.equal(getScheme()); | ||
}); | ||
}); | ||
describe('static getAllSchemes()', () => { | ||
it('is equivalent to getInstance().getAllSchemes()', () => { | ||
expect(getInstance().getAllSchemes()).to.equal(getAllSchemes()); | ||
}); | ||
}); | ||
describe('static getDefaultSchemeName()', () => { | ||
it('is equivalent to getInstance().getDefaultSchemeName()', () => { | ||
expect(getInstance().getDefaultSchemeName()).to.equal(getDefaultSchemeName()); | ||
}); | ||
}); | ||
describe('static setDefaultSchemeName()', () => { | ||
it('is equivalent to getInstance().setDefaultSchemeName()', () => { | ||
setDefaultSchemeName('test2'); | ||
const name = getInstance().getDefaultSchemeName(); | ||
expect(name).to.equal('test2'); | ||
setDefaultSchemeName('test'); | ||
}); | ||
}); | ||
describe('static registerScheme()', () => { | ||
it('is equivalent to getInstance().registerScheme()', () => { | ||
registerScheme('test3', ['cyan', 'magenta']); | ||
const scheme = getInstance().getScheme('test3'); | ||
expect(scheme).to.deep.equal(['cyan', 'magenta']); | ||
}); | ||
}); | ||
describe('static registerMultipleSchemes()', () => { | ||
it('is equivalent to getInstance().registerMultipleSchemes()', () => { | ||
registerMultipleSchemes({ | ||
test4: ['cyan', 'magenta'], | ||
test5: ['brown', 'purple'], | ||
}); | ||
const scheme1 = getInstance().getScheme('test4'); | ||
expect(scheme1).to.deep.equal(['cyan', 'magenta']); | ||
const scheme2 = getInstance().getScheme('test5'); | ||
expect(scheme2).to.deep.equal(['brown', 'purple']); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.