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

feat(color): support analogous colors to prevent color conflict #19325

Merged
merged 5 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { ExtensibleFunction } from '../models';
import { ColorsLookup } from './types';
import stringifyAndTrim from './stringifyAndTrim';
import getSharedLabelColor from './SharedLabelColorSingleton';
import { getAnalogousColors } from './utils';

// Use type augmentation to correct the fact that
// an instance of CategoricalScale is also a function
Expand All @@ -31,6 +32,8 @@ interface CategoricalColorScale {
}

class CategoricalColorScale extends ExtensibleFunction {
originColors: string[];

colors: string[];

scale: ScaleOrdinal<{ toString(): string }, string>;
Expand All @@ -39,6 +42,8 @@ class CategoricalColorScale extends ExtensibleFunction {

forcedColors: ColorsLookup;

multiple: number;

/**
* Constructor
* @param {*} colors an array of colors
Expand All @@ -48,11 +53,13 @@ class CategoricalColorScale extends ExtensibleFunction {
constructor(colors: string[], parentForcedColors?: ColorsLookup) {
super((value: string, sliceId?: number) => this.getColor(value, sliceId));

this.originColors = colors;
this.colors = colors;
this.scale = scaleOrdinal<{ toString(): string }, string>();
this.scale.range(colors);
this.parentForcedColors = parentForcedColors;
this.forcedColors = {};
this.multiple = 0;
}

getColor(value?: string, sliceId?: number) {
Expand All @@ -72,6 +79,15 @@ class CategoricalColorScale extends ExtensibleFunction {
return forcedColor;
}

const multiple = Math.floor(
this.domain().length / this.originColors.length,
);
if (multiple > this.multiple) {
this.multiple = multiple;
const newRange = getAnalogousColors(this.originColors, multiple);
this.range(this.originColors.concat(newRange));
}

const color = this.scale(cleanedValue);
sharedLabelColor.addSlice(cleanedValue, color, sliceId);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
* under the License.
*/

import tinycolor from 'tinycolor2';
import { CategoricalColorNamespace } from '.';
import makeSingleton from '../utils/makeSingleton';
import { getAnalogousColors } from './utils';

export class SharedLabelColor {
sliceLabelColorMap: Record<number, Record<string, string | undefined>>;
Expand All @@ -39,27 +39,16 @@ export class SharedLabelColor {
CategoricalColorNamespace.getNamespace(colorNamespace);
const colors = categoricalNamespace.getScale(colorScheme).range();
const sharedLabels = this.getSharedLabels();
const generatedColors: tinycolor.Instance[] = [];
let generatedColors: string[] = [];
let sharedLabelMap;

if (sharedLabels.length) {
const multiple = Math.ceil(sharedLabels.length / colors.length);
const ext = 5;
const analogousColors = colors.map(color => {
const result = tinycolor(color).analogous(multiple + ext);
return result.slice(ext);
});

// [[A, AA, AAA], [B, BB, BBB]] => [A, B, AA, BB, AAA, BBB]
while (analogousColors[analogousColors.length - 1]?.length) {
analogousColors.forEach(colors =>
generatedColors.push(colors.shift() as tinycolor.Instance),
);
}
generatedColors = getAnalogousColors(colors, multiple);
sharedLabelMap = sharedLabels.reduce(
(res, label, index) => ({
...res,
[label.toString()]: generatedColors[index]?.toHexString(),
[label.toString()]: generatedColors[index],
}),
{},
);
Expand Down
22 changes: 22 additions & 0 deletions superset-frontend/packages/superset-ui-core/src/color/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import tinycolor from 'tinycolor2';

const rgbRegex = /^rgb\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\)$/;
export function getContrastingColor(color: string, thresholds = 186) {
Expand Down Expand Up @@ -51,3 +52,24 @@ export function getContrastingColor(color: string, thresholds = 186) {

return r * 0.299 + g * 0.587 + b * 0.114 > thresholds ? '#000' : '#FFF';
}

export function getAnalogousColors(colors: string[], results: number) {
const generatedColors: string[] = [];
// This is to solve the problem that the first three values generated by tinycolor.analogous
// may have the same or very close colors.
const ext = 3;
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This is to solve the problem that the first three values generated by tinycolor.analogous may have the same or very close colors. You can see my local test:
image
Let me add some comments here.

const analogousColors = colors.map(color => {
const result = tinycolor(color).analogous(results + ext);
return result.slice(ext);
});

// [[A, AA, AAA], [B, BB, BBB]] => [A, B, AA, BB, AAA, BBB]
Copy link
Member

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.

while (analogousColors[analogousColors.length - 1]?.length) {
analogousColors.forEach(colors => {
const color = colors.shift() as tinycolor.Instance;
generatedColors.push(color.toHexString());
});
}

return generatedColors;
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,28 +62,15 @@ describe('CategoricalColorScale', () => {
expect(c2).not.toBe(c3);
expect(c3).not.toBe(c1);
});
it('recycles colors when number of items exceed available colors', () => {
const colorSet: { [key: string]: number } = {};
it('get analogous colors when number of items exceed available colors', () => {
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] += 1;
} else {
colorSet[color] = 1;
}
});
expect(Object.keys(colorSet)).toHaveLength(3);
['blue', 'red', 'green'].forEach(color => {
expect(colorSet[color]).toBe(2);
});
scale.getColor('pig');
scale.getColor('horse');
scale.getColor('cat');
scale.getColor('cow');
scale.getColor('donkey');
scale.getColor('goat');
expect(scale.range()).toHaveLength(6);
});
});
describe('.setColor(value, forcedColor)', () => {
Expand Down