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

Improve categorical color management #5815

Merged
merged 33 commits into from
Sep 12, 2018
Merged
Show file tree
Hide file tree
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 Sep 1, 2018
385a21a
verify to pass existing unit tests
kristw Sep 1, 2018
1502fcb
separate logic for forcing color and getting color
kristw Sep 5, 2018
1fc211d
replace getColorFromScheme with CategoricalColorManager
kristw Sep 5, 2018
e3ddbd5
organize static functions
kristw Sep 5, 2018
dd60dc7
migrate to new function
kristw Sep 5, 2018
211a777
Remove ALL_COLOR_SCHEMES
kristw Sep 5, 2018
c1aeeb5
move sequential colors to another file
kristw Sep 5, 2018
56f9ef3
extract categorical colors to separate file
kristw Sep 5, 2018
f3671c8
move airbnb and lyft colors to separate files
kristw Sep 5, 2018
9894368
fix missing toFunction()
kristw Sep 5, 2018
943ecac
Rewrite to support local and global force items, plus namespacing.
kristw Sep 7, 2018
3300080
fix references
kristw Sep 7, 2018
2efac2e
revert nvd3
kristw Sep 7, 2018
1515f05
update namespace api
kristw Sep 8, 2018
cd48bd6
Update the visualizations
kristw Sep 8, 2018
2915afc
update usage with static functions
kristw Sep 8, 2018
d60d4ee
update unit test
kristw Sep 8, 2018
f456065
add unit test
kristw Sep 8, 2018
75318f4
rename default namespace
kristw Sep 8, 2018
9d457a0
add unit test for color namespace
kristw Sep 10, 2018
00d8090
add unit test for namespace
kristw Sep 10, 2018
8a91c4d
start unit test for colorschememanager
kristw Sep 10, 2018
47c5f20
add unit tests for color scheme manager
kristw Sep 11, 2018
e2dba45
check returns for chaining
kristw Sep 11, 2018
500f441
complete unit test for the new classes
kristw Sep 11, 2018
1312dd5
fix color tests
kristw Sep 11, 2018
9dbccd6
update unit tests
kristw Sep 11, 2018
d08be39
update unit tests
kristw Sep 11, 2018
06970de
move color scheme registration to common
kristw Sep 12, 2018
527e244
update unit test
kristw Sep 12, 2018
1cb5d61
rename sharedForcedColors to parentForcedColors
kristw Sep 12, 2018
1cf4695
remove import
kristw Sep 12, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset/assets/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"test": "spec"
},
"scripts": {
"tdd": "mocha --require ignore-styles --compilers js:babel-core/register --require spec/helpers/shim.js 'spec/**/*_spec.*' --watch --recursive",
"test": "mocha --require ignore-styles --compilers js:babel-core/register --require spec/helpers/shim.js 'spec/**/*_spec.*'",
"test:one": "mocha --require ignore-styles --compilers js:babel-core/register --require spec/helpers/shim.js",
"cover": "babel-node node_modules/.bin/babel-istanbul cover _mocha -- --compilers babel-core/register --require spec/helpers/shim.js --require ignore-styles 'spec/**/*_spec.*'",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import { Creatable } from 'react-select';

import ColorSchemeControl from
'../../../../src/explore/components/controls/ColorSchemeControl';
import { ALL_COLOR_SCHEMES } from '../../../../src/modules/colors';
import { getAllSchemes } from '../../../../src/modules/ColorSchemeManager';

const defaultProps = {
options: Object.keys(ALL_COLOR_SCHEMES).map(s => ([s, s])),
options: Object.keys(getAllSchemes()).map(s => ([s, s])),
};

describe('ColorSchemeControl', () => {
Expand Down
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);
});
});
});
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', () => {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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 superset/assets/spec/javascripts/modules/ColorSchemeManager_spec.js
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']);
});
});
});
Loading