geido commented on code in PR #27439:
URL: https://github.com/apache/superset/pull/27439#discussion_r1518057877
##########
superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts:
##########
@@ -39,99 +38,172 @@ class CategoricalColorScale extends ExtensibleFunction {
scale: ScaleOrdinal<{ toString(): string }, string>;
- parentForcedColors: ColorsLookup;
-
forcedColors: ColorsLookup;
+ sharedColorMapInstance: ReturnType<typeof getSharedLabelColor>;
+
+ sliceMap: Map<string, string>;
+
multiple: number;
/**
* Constructor
* @param {*} colors an array of colors
- * @param {*} parentForcedColors optional parameter that comes from parent
- * (usually CategoricalColorNamespace) and supersede this.forcedColors
+ * @param {*} forcedColors optional parameter that comes from parent
+ * (usually CategoricalColorNamespace)
*/
- constructor(colors: string[], parentForcedColors: ColorsInitLookup = {}) {
+ constructor(colors: string[], forcedColors: ColorsInitLookup = {}) {
super((value: string, sliceId?: number) => this.getColor(value, sliceId));
-
+ // holds original color scheme colors
this.originColors = colors;
+ // holds the extended color range (includes analagous colors)
this.colors = colors;
+ // holds the values of this specific slice (label+color)
+ this.sliceMap = new Map();
+ // shared color map instance (when context is shared, i.e. dashboard)
+ this.sharedColorMapInstance = getSharedLabelColor();
+ // holds the multiple value for analogous colors range
+ this.multiple = 0;
+
this.scale = scaleOrdinal<{ toString(): string }, string>();
this.scale.range(colors);
// reserve fixed colors in parent map based on their index in the scale
- Object.entries(parentForcedColors).forEach(([key, value]) => {
+ Object.entries(forcedColors).forEach(([key, value]) => {
if (typeof value === 'number') {
// eslint-disable-next-line no-param-reassign
- parentForcedColors[key] = colors[value % colors.length];
+ forcedColors[key] = colors[value % colors.length];
}
});
- // all indexes have been replaced by a fixed color
- this.parentForcedColors = parentForcedColors as ColorsLookup;
- this.forcedColors = {};
- this.multiple = 0;
+ // forced colors from parent (usually CategoricalColorNamespace)
+ // currently used in dashboards to set custom label colors
+ this.forcedColors = forcedColors as ColorsLookup;
}
- removeSharedLabelColorFromRange(
- sharedColorMap: Map<string, string>,
- cleanedValue: string,
- ) {
- // make sure we don't overwrite the origin colors
- const updatedRange = new Set(this.originColors);
- // remove the color option from shared color
- sharedColorMap.forEach((value: string, key: string) => {
- if (key !== cleanedValue) {
- updatedRange.delete(value);
- }
- });
- // remove the color option from forced colors
- Object.entries(this.parentForcedColors).forEach(([key, value]) => {
- if (key !== cleanedValue) {
- updatedRange.delete(value);
- }
- });
- this.range(updatedRange.size > 0 ? [...updatedRange] : this.originColors);
+ /**
+ * Increments the color range with analogous colors
+ */
+ incrementColorRange() {
+ const multiple = Math.floor(
+ this.domain().length / this.originColors.length,
+ );
+ // the domain has grown larger than the original range
+ // increments the range with analogous colors
+ if (multiple > this.multiple) {
+ this.multiple = multiple;
+ const newRange = getAnalogousColors(this.originColors, multiple);
+ const extendedColors = this.originColors.concat(newRange);
+
+ this.range(extendedColors);
+ this.colors = extendedColors;
+ }
}
- getColor(value?: string, sliceId?: number) {
+ /**
+ * Get the color for a given value
+ * @param value
+ * @param sliceId
+ * @returns the color for the given value
+ */
+ getColor(value?: string, sliceId?: number): string {
const cleanedValue = stringifyAndTrim(value);
- const sharedLabelColor = getSharedLabelColor();
- const sharedColorMap = sharedLabelColor.getColorMap();
+ const sharedColorMap = this.sharedColorMapInstance.getColorMap();
const sharedColor = sharedColorMap.get(cleanedValue);
-
- // priority: parentForcedColors > forcedColors > labelColors
- let color =
- this.parentForcedColors?.[cleanedValue] ||
- this.forcedColors?.[cleanedValue] ||
- sharedColor;
-
- if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) {
- 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));
+ // priority: forced color (i.e. custom label colors) > shared color >
scale color
+ const forcedColor = this.forcedColors?.[cleanedValue] || sharedColor;
+ const isExistingLabel = this.sliceMap.has(cleanedValue);
+ let color = forcedColor || this.scale(cleanedValue);
+
+ // a forced color will always be used independently of the usage count
+ if (!forcedColor && !isExistingLabel) {
+ if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) {
+ this.incrementColorRange();
}
- }
- const newColor = this.scale(cleanedValue);
- if (!color) {
- color = newColor;
- if (isFeatureEnabled(FeatureFlag.AvoidColorsCollision)) {
- this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue);
- color = this.scale(cleanedValue);
+ if (
+ // feature flag to be deprecated (will become standard behaviour)
+ isFeatureEnabled(FeatureFlag.AvoidColorsCollision) &&
+ this.isColorUsed(color)
+ ) {
+ // fallback to least used color
+ color = this.getNextAvailableColor(color);
}
}
- sharedLabelColor.addSlice(cleanedValue, color, sliceId);
+ // keep track of values in this slice
+ this.sliceMap.set(cleanedValue, color);
+
+ // store the value+color in the shared context
+ if (sliceId) {
+ this.sharedColorMapInstance.addSlice(cleanedValue, color, sliceId);
+ }
return color;
}
/**
- * Enforce specific color for given value
+ *
+ * @param color
+ * @returns whether the color is used in this slice
+ */
+ isColorUsed(color: string): boolean {
+ return this.getColorUsageCount(color) > 0;
+ }
+
+ /**
+ * Get the count of the color usage in this slice
+ * @param sliceId
+ * @param color
+ * @returns the count of the color usage in this slice
+ */
+ getColorUsageCount(currentColor: string): number {
+ let count = 0;
+ this.sliceMap.forEach(color => {
+ if (color === currentColor) {
+ count += 1;
+ }
+ });
+ return count;
+ }
+
+ /**
+ * Lower chances of color collision by returning the least used color
+ * Checks across colors of current slice within SharedLabelColorSingleton
+ *
+ * @param currentColor
+ * @returns the least used color that is not the excluded color
+ */
+ getNextAvailableColor(currentColor: string) {
Review Comment:
This is meant to help with color collisions. It orders the colors based on
their usage count in the chart. Least used colors will be preferred. This does
not guarantee full protection against color collisions, which should be
implemented at the individual viz based on their shape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
