You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2024/03/08 15:08:49 UTC
(superset) branch diego/ch79154/fix-color-inconsistency created (now 6f89a9ca0e)
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a change to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
at 6f89a9ca0e Update map on hidden tabs
This branch includes the following new commits:
new 91f0d51abd Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
new 43fe16227d Prefer less used colors
new 51bf74f0ed Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
new 6f89a9ca0e Update map on hidden tabs
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 03/04: Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
Posted by di...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 51bf74f0ed9d606a2fdc380ca47403967baccf9d
Merge: 43fe16227d f453d5d7e7
Author: geido <di...@gmail.com>
AuthorDate: Fri Mar 1 20:59:12 2024 +0200
Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
.../creating-charts-dashboards/exploring-data.mdx | 27 ----------------------
docs/package.json | 2 +-
docs/yarn.lock | 8 +++----
requirements/base.in | 1 +
requirements/base.txt | 8 ++++---
.../legacy-plugin-chart-heatmap/src/Heatmap.js | 15 +++++-------
.../src/transformProps.js | 8 +++++--
.../src/SqlLab/components/SouthPane/Results.tsx | 2 +-
.../SqlLab/components/SouthPane/SouthPane.test.tsx | 13 +++++++++++
.../src/SqlLab/components/SouthPane/index.tsx | 14 +++++------
superset/security/manager.py | 1 +
tests/integration_tests/security_tests.py | 1 +
12 files changed, 45 insertions(+), 55 deletions(-)
(superset) 04/04: Update map on hidden tabs
Posted by di...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6f89a9ca0e0808cdf0b9a564db65f09c9e20d11e
Author: geido <di...@gmail.com>
AuthorDate: Fri Mar 8 16:08:34 2024 +0100
Update map on hidden tabs
---
RESOURCES/FEATURE_FLAGS.md | 1 +
.../cypress/e2e/dashboard/editmode.test.ts | 56 ++++---
.../src/color/CategoricalColorScale.ts | 149 +++++++++++-------
.../src/color/SharedLabelColorSingleton.ts | 19 +--
.../test/color/CategoricalColorNameSpace.test.ts | 2 +-
.../test/color/CategoricalColorScale.test.ts | 167 ++++++++++-----------
.../test/color/SharedLabelColorSingleton.test.ts | 16 +-
.../src/dashboard/actions/dashboardInfo.ts | 13 +-
.../src/dashboard/components/Header/index.jsx | 10 +-
.../dashboard/components/PropertiesModal/index.tsx | 7 +-
.../dashboard/components/gridComponents/Tabs.jsx | 51 ++++++-
.../dashboard/containers/DashboardComponent.jsx | 1 +
.../src/dashboard/containers/DashboardPage.tsx | 1 +
.../components/ExploreChartHeader/index.jsx | 23 ++-
tests/integration_tests/superset_test_config.py | 1 +
15 files changed, 296 insertions(+), 221 deletions(-)
diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md
index 119988653d..1bbf24f4be 100644
--- a/RESOURCES/FEATURE_FLAGS.md
+++ b/RESOURCES/FEATURE_FLAGS.md
@@ -84,6 +84,7 @@ These features flags currently default to True and **will be removed in a future
[//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY"
+- AVOID_COLORS_COLLISION
- DASHBOARD_CROSS_FILTERS
- ENABLE_JAVASCRIPT_CONTROLS
- KV_STORE
diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
index 28d5c166f5..e50bf52be9 100644
--- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
+++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
@@ -19,7 +19,12 @@
import { SAMPLE_DASHBOARD_1, TABBED_DASHBOARD } from 'cypress/utils/urls';
import { drag, resize, waitForChartLoad } from 'cypress/utils';
import * as ace from 'brace';
-import { interceptGet, interceptUpdate, openTab } from './utils';
+import {
+ interceptGet,
+ interceptUpdate,
+ openTab,
+ interceptGet as interceptDashboardGet,
+} from './utils';
import {
interceptExploreJson,
interceptFiltering as interceptCharts,
@@ -124,7 +129,7 @@ function selectColorScheme(color: string) {
)
.first()
.click();
- cy.getBySel(color).click();
+ cy.getBySel(color).click({ force: true });
}
function applyChanges() {
@@ -169,6 +174,7 @@ function writeMetadata(metadata: string) {
function openExplore(chartName: string) {
interceptExploreJson();
+ interceptDashboardGet();
cy.get(
`[data-test-chart-name='${chartName}'] [aria-label='More Options']`,
@@ -210,7 +216,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
});
it('should apply same color to same labels with color scheme set', () => {
@@ -231,7 +237,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open 2nd main tab
openTab(0, 1);
@@ -240,7 +246,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
});
it('should apply same color to same labels with no color scheme set', () => {
@@ -261,7 +267,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
// open 2nd main tab
openTab(0, 1);
@@ -270,7 +276,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
});
it('custom label colors should take the precedence in nested tabs', () => {
@@ -384,17 +390,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
- .should('have.css', 'fill', 'rgb(224, 67, 85)');
+ .should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
- .should('have.css', 'fill', 'rgb(163, 143, 121)');
+ .should('have.css', 'fill', 'rgb(90, 193, 137)');
openProperties();
cy.get('[aria-label="Select color scheme"]').should('have.value', '');
@@ -423,17 +429,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
- .should('have.css', 'fill', 'rgb(224, 67, 85)');
+ .should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
- .should('have.css', 'fill', 'rgb(163, 143, 121)');
+ .should('have.css', 'fill', 'rgb(90, 193, 137)');
});
it('should show the same colors in Explore', () => {
@@ -459,12 +465,6 @@ describe('Dashboard edit', () => {
)
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
- // label Christopher
- cy.get(
- '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
- )
- .eq(1)
- .should('have.css', 'fill', 'rgb(172, 32, 119)');
openExplore('Top 10 California Names Timeseries');
@@ -472,10 +472,6 @@ describe('Dashboard edit', () => {
cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
- // label Christopher
- cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
- .eq(1)
- .should('have.css', 'fill', 'rgb(172, 32, 119)');
});
it('should change color scheme multiple times', () => {
@@ -496,7 +492,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open 2nd main tab
openTab(0, 1);
@@ -505,7 +501,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
editDashboard();
openProperties();
@@ -516,7 +512,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(244, 176, 42)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
// open main tab and nested tab
openTab(0, 0);
@@ -527,7 +523,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(244, 176, 42)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
});
it('should apply the color scheme across main tabs', () => {
@@ -542,7 +538,7 @@ describe('Dashboard edit', () => {
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
});
it('should apply the color scheme across main tabs for rendered charts', () => {
@@ -558,7 +554,7 @@ describe('Dashboard edit', () => {
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
- .should('have.css', 'fill', 'rgb(156, 52, 152)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
// change scheme now that charts are rendered across the main tabs
editDashboard();
@@ -588,7 +584,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open another nested tab
openTab(2, 1);
diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
index c4e96a9a1f..4223fae776 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
@@ -17,11 +17,13 @@ class CategoricalColorScale extends ExtensibleFunction {
colors: string[];
+ expandedColors: string[];
+
scale: ScaleOrdinal<{ toString(): string }, string>;
forcedColors: ColorsLookup;
- colorUsageCount: Map<string, number>;
+ sharedColorMapInstance: ReturnType<typeof getSharedLabelColor>;
sliceMap: Map<string, string>;
@@ -35,8 +37,17 @@ class CategoricalColorScale extends ExtensibleFunction {
*/
constructor(colors: string[], forcedColors: ColorsInitLookup = {}) {
super((value: string, sliceId?: number) => this.getColor(value, sliceId));
+ // holds original color scheme colors
this.originColors = colors;
- this.colors = colors;
+ // holds the extended color range (includes analagous colors)
+ this.expandedColors = 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);
@@ -49,18 +60,12 @@ class CategoricalColorScale extends ExtensibleFunction {
});
// forced colors from parent (usually CategoricalColorNamespace)
+ // currently used in dashboards to set custom label colors
this.forcedColors = forcedColors as ColorsLookup;
- // holds the usage count for each color including forced colors from parent
- this.colorUsageCount = this.initColorUsageCounter();
- // holds the values in this specific slice
- this.sliceMap = new Map();
- // holds the multiple value for analogous colors range
- this.multiple = 0;
}
/**
* Increments the color range with analogous colors
- *
*/
incrementColorRange() {
const multiple = Math.floor(
@@ -71,34 +76,24 @@ class CategoricalColorScale extends ExtensibleFunction {
if (multiple > this.multiple) {
this.multiple = multiple;
const newRange = getAnalogousColors(this.originColors, multiple);
- this.range(this.originColors.concat(newRange));
- // update the colorUsageCount map with the new colors
- newRange.forEach(color => {
- this.colorUsageCount.set(color, 0);
- });
+ const expandedColors = this.originColors.concat(newRange);
+
+ this.range(expandedColors);
+ this.expandedColors = expandedColors;
}
}
/**
- * Initializes the color usage count map
- * @returns a map of color to usage count
+ * Get the color for a given value
+ * @param value
+ * @param sliceId
+ * @returns the color for the given value
*/
- initColorUsageCounter(): Map<string, number> {
- const colorUsageCount = new Map(this.colors.map(color => [color, 0]));
- // add forced colors to the usage count map
- // helps reducing conflicts but increases randomness
- Object.values(this.forcedColors).forEach(color => {
- const count = colorUsageCount.get(color) || 0;
- colorUsageCount.set(color, count + 1);
- });
- return colorUsageCount;
- }
-
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: forced color (example: custom label colors) > shared color > scale color
const forcedColor = this.forcedColors?.[cleanedValue] || sharedColor;
let color = forcedColor || this.scale(cleanedValue);
@@ -107,22 +102,24 @@ class CategoricalColorScale extends ExtensibleFunction {
if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) {
this.incrementColorRange();
}
- if (this.isColorUsed(color)) {
- // color was used in this slice already
+ 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);
}
}
- // increment the usage count for the color
- this.incrementColorUsage(color);
-
- // store the value+color in the shared map for dashboard consistency
- 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;
}
@@ -132,30 +129,69 @@ class CategoricalColorScale extends ExtensibleFunction {
* @returns whether the color is used in this slice
*/
isColorUsed(color: string): boolean {
- return Array.from(this.sliceMap.values()).includes(color);
+ 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 and forced colors (all slices from parent)
+ * Checks across colors of current slice within SharedLabelColorSingleton
*
- * @param excludeColor
+ * @param currentColor
* @returns the least used color that is not the excluded color
*/
- getNextAvailableColor(excludeColor: string) {
- // sort the colors by count, then exclude the specified color
- const sortedColors = [...this.colorUsageCount.entries()]
- .sort((a, b) => a[1] - b[1])
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
- .filter(([color, _]) => color !== excludeColor);
- const color = sortedColors[0][0];
+ getNextAvailableColor(currentColor: string) {
+ const colorUsageArray = this.expandedColors.map(color => ({
+ color,
+ count: this.getColorUsageCount(color),
+ }));
+ const currentColorCount = this.getColorUsageCount(currentColor);
+ const otherColors = colorUsageArray.filter(
+ colorEntry => colorEntry.color !== currentColor,
+ );
+ // all other colors are used as much or more than currentColor
+ const hasNoneAvailable = otherColors.every(
+ colorEntry => colorEntry.count >= currentColorCount,
+ );
- return color;
+ // fallback to currentColor color
+ if (!otherColors.length || hasNoneAvailable) {
+ return currentColor;
+ }
+
+ // Finding the least used color
+ const leastUsedColor = otherColors.reduce((min, entry) =>
+ entry.count < min.count ? entry : min,
+ ).color;
+
+ return leastUsedColor;
}
- incrementColorUsage(color: string) {
- const currentCount = this.colorUsageCount.get(color) || 0;
- this.colorUsageCount.set(color, currentCount + 1);
+ /**
+ * Enforce specific color for a given value at the scale level
+ * Overrides any existing color and forced color for the given value
+ *
+ * @param {*} value value
+ * @param {*} forcedColor forcedColor
+ */
+ setColor(value: string, forcedColor: string) {
+ this.forcedColors[stringifyAndTrim(value)] = forcedColor;
+ return this;
}
/**
@@ -163,11 +199,15 @@ class CategoricalColorScale extends ExtensibleFunction {
* @returns an object where the key is the data value and the value is the hex color code
*/
getColorMap() {
- const colorMap = {};
+ const colorMap: { [key: string]: string | undefined } = {};
this.scale.domain().forEach(value => {
colorMap[value.toString()] = this.scale(value);
});
- return { ...colorMap, ...this.forcedColors };
+
+ return {
+ ...colorMap,
+ ...this.forcedColors,
+ };
}
/**
@@ -179,7 +219,6 @@ class CategoricalColorScale extends ExtensibleFunction {
this.forcedColors,
);
copy.forcedColors = { ...this.forcedColors };
- copy.colorUsageCount = new Map(this.colorUsageCount);
copy.domain(this.domain());
copy.unknown(this.unknown());
return copy;
@@ -225,7 +264,7 @@ class CategoricalColorScale extends ExtensibleFunction {
return this.scale.range();
}
- this.colors = newRange;
+ this.expandedColors = newRange;
this.scale.range(newRange);
return this;
}
diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
index 4837d0f362..05a56de129 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
@@ -43,10 +43,10 @@ export class SharedLabelColor {
CategoricalColorNamespace.getNamespace(colorNamespace);
const newColorMap = new Map();
this.colorMap.clear();
- this.sliceLabelMap.forEach(labels => {
+ this.sliceLabelMap.forEach((labels, sliceId) => {
const colorScale = categoricalNamespace.getScale(colorScheme);
labels.forEach(label => {
- const newColor = colorScale(label);
+ const newColor = colorScale.getColor(label, sliceId);
newColorMap.set(label, newColor);
});
});
@@ -57,12 +57,8 @@ export class SharedLabelColor {
return this.colorMap;
}
- addSlice(label: string, color: string, sliceId?: number) {
- if (
- this.source !== SharedLabelColorSource.Dashboard ||
- sliceId === undefined
- )
- return;
+ addSlice(label: string, color: string, sliceId: number) {
+ if (this.source !== SharedLabelColorSource.Dashboard) return;
const labels = this.sliceLabelMap.get(sliceId) || [];
if (!labels.includes(label)) {
labels.push(label);
@@ -83,13 +79,6 @@ export class SharedLabelColor {
this.colorMap = newColorMap;
}
- reset() {
- const copyColorMap = new Map(this.colorMap);
- copyColorMap.forEach((_, label) => {
- this.colorMap.set(label, '');
- });
- }
-
clear() {
this.sliceLabelMap.clear();
this.colorMap.clear();
diff --git a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
index d5e1ef8f0b..69fb38eea3 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
@@ -98,7 +98,7 @@ describe('CategoricalColorNamespace', () => {
namespace.setColor('dog', 'black');
const scale = namespace.getScale('testColors');
scale.setColor('dog', 'pink');
- expect(scale.getColor('dog')).toBe('black');
+ expect(scale.getColor('dog')).toBe('pink');
expect(scale.getColor('boy')).not.toBe('black');
});
it('does not affect scales in other namespaces', () => {
diff --git a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
index 48e43d8351..987c83fbc5 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
@@ -18,50 +18,49 @@
*/
import { ScaleOrdinal } from 'd3-scale';
-import {
- CategoricalColorScale,
- FeatureFlag,
- getSharedLabelColor,
-} from '@superset-ui/core';
+import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core';
describe('CategoricalColorScale', () => {
it('exists', () => {
expect(CategoricalColorScale !== undefined).toBe(true);
});
- describe('new CategoricalColorScale(colors, parentForcedColors)', () => {
- it('can create new scale when parentForcedColors is not given', () => {
+ describe('new CategoricalColorScale(colors, forcedColors)', () => {
+ it('can create new scale when forcedColors is not given', () => {
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
expect(scale).toBeInstanceOf(CategoricalColorScale);
});
- it('can create new scale when parentForcedColors is given', () => {
- const parentForcedColors = {};
+ it('can create new scale when forcedColors is given', () => {
+ const forcedColors = {};
const scale = new CategoricalColorScale(
['blue', 'red', 'green'],
- parentForcedColors,
+ forcedColors,
);
expect(scale).toBeInstanceOf(CategoricalColorScale);
- expect(scale.parentForcedColors).toBe(parentForcedColors);
+ expect(scale.forcedColors).toBe(forcedColors);
});
it('can refer to colors based on their index', () => {
- const parentForcedColors = { pig: 1, horse: 5 };
+ const forcedColors = { pig: 1, horse: 5 };
const scale = new CategoricalColorScale(
['blue', 'red', 'green'],
- parentForcedColors,
+ forcedColors,
);
expect(scale.getColor('pig')).toEqual('red');
- expect(parentForcedColors.pig).toEqual('red');
+ expect(forcedColors.pig).toEqual('red');
// can loop around the scale
expect(scale.getColor('horse')).toEqual('green');
- expect(parentForcedColors.horse).toEqual('green');
+ expect(forcedColors.horse).toEqual('green');
});
});
describe('.getColor(value)', () => {
it('returns same color for same value', () => {
- const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ const scale = new CategoricalColorScale(['blue', 'red', 'green'], {
+ pig: 'red',
+ horse: 'green',
+ });
const c1 = scale.getColor('pig');
const c2 = scale.getColor('horse');
const c3 = scale.getColor('pig');
@@ -118,26 +117,8 @@ describe('CategoricalColorScale', () => {
scale.getColor('cow');
scale.getColor('donkey');
scale.getColor('goat');
- expect(scale.range()).toHaveLength(6);
+ expect(scale.range()).toHaveLength(9);
});
-
- it('should remove shared color from range if avoid colors collision enabled', () => {
- window.featureFlags = {
- [FeatureFlag.AvoidColorsCollision]: true,
- };
- const scale = new CategoricalColorScale(['blue', 'red', 'green']);
- const color1 = scale.getColor('a', 1);
- expect(scale.range()).toHaveLength(3);
- const color2 = scale.getColor('a', 2);
- expect(color1).toBe(color2);
- scale.getColor('b', 2);
- expect(scale.range()).toHaveLength(2);
- scale.getColor('c', 2);
- expect(scale.range()).toHaveLength(1);
- });
- window.featureFlags = {
- [FeatureFlag.AvoidColorsCollision]: false,
- };
});
describe('.setColor(value, forcedColor)', () => {
it('overrides default color', () => {
@@ -145,16 +126,14 @@ describe('CategoricalColorScale', () => {
scale.setColor('pig', 'pink');
expect(scale.getColor('pig')).toBe('pink');
});
- it('does not override parentForcedColors', () => {
+ it('does override forcedColors', () => {
const scale1 = new CategoricalColorScale(['blue', 'red', 'green']);
scale1.setColor('pig', 'black');
- const scale2 = new CategoricalColorScale(
- ['blue', 'red', 'green'],
- scale1.forcedColors,
- );
+
+ const scale2 = new CategoricalColorScale(['blue', 'red', 'green']);
scale2.setColor('pig', 'pink');
+ expect(scale2.getColor('pig')).toBe('pink');
expect(scale1.getColor('pig')).toBe('black');
- expect(scale2.getColor('pig')).toBe('black');
});
it('returns the scale', () => {
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
@@ -163,7 +142,7 @@ describe('CategoricalColorScale', () => {
});
});
describe('.getColorMap()', () => {
- it('returns correct mapping and parentForcedColors and forcedColors are specified', () => {
+ it('returns correct mapping using least used color', () => {
const scale1 = new CategoricalColorScale(['blue', 'red', 'green']);
scale1.setColor('cow', 'black');
const scale2 = new CategoricalColorScale(
@@ -177,7 +156,7 @@ describe('CategoricalColorScale', () => {
expect(scale2.getColorMap()).toEqual({
cow: 'black',
pig: 'pink',
- horse: 'green',
+ horse: 'blue', // least used color
});
});
});
@@ -230,64 +209,72 @@ describe('CategoricalColorScale', () => {
});
describe('a CategoricalColorScale instance is also a color function itself', () => {
- it('scale(value) returns color similar to calling scale.getColor(value)', () => {
+ it('scale(value) returns least used color', () => {
+ window.featureFlags = {
+ [FeatureFlag.AvoidColorsCollision]: true,
+ };
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
- expect(scale.getColor('pig')).toBe(scale('pig'));
- expect(scale.getColor('cat')).toBe(scale('cat'));
+ expect(scale.getColor('pig')).toBe('blue');
+ expect(scale('pig')).toBe('red'); // blue is now used and red is next available
+ expect(scale.getColor('cat')).toBe('blue'); // pig got red, now blue is available
+ expect(scale('cat')).toBe('green'); // blue is now used and green is next available
});
- });
-
- describe("is compatible with D3's ScaleOrdinal", () => {
- it('passes type check', () => {
- const scale: ScaleOrdinal<{ toString(): string }, string> =
- new CategoricalColorScale(['blue', 'red', 'green']);
+ it('scale(value) returns same color for same value', () => {
+ window.featureFlags = {
+ [FeatureFlag.AvoidColorsCollision]: false,
+ };
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ expect(scale.getColor('pig')).toBe('blue');
expect(scale('pig')).toBe('blue');
+ expect(scale.getColor('cat')).toBe('red');
+ expect(scale('cat')).toBe('red');
});
});
- describe('.removeSharedLabelColorFromRange(colorMap, cleanedValue)', () => {
- it('should remove shared color from range', () => {
- const scale = new CategoricalColorScale(['blue', 'green', 'red']);
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
+ describe('.getNextAvailableColor(currentColor)', () => {
+ it('returns the current color if it is the least used or equally used among colors', () => {
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ scale.getColor('cat');
+ scale.getColor('dog');
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.clear();
- const colorMap = sharedLabelColor.getColorMap();
- sharedLabelColor.addSlice('cow', 'blue', 1);
- scale.removeSharedLabelColorFromRange(colorMap, 'pig');
- expect(scale.range()).toEqual(['green', 'red']);
- scale.removeSharedLabelColorFromRange(colorMap, 'cow');
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- sharedLabelColor.clear();
+ // Since 'green' hasn't been used, it's considered the least used.
+ expect(scale.getNextAvailableColor('blue')).toBe('green');
});
- it('recycles colors when all colors are in sharedLabelColor', () => {
- const scale = new CategoricalColorScale(['blue', 'green', 'red']);
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- const sharedLabelColor = getSharedLabelColor();
- const colorMap = sharedLabelColor.getColorMap();
- sharedLabelColor.addSlice('cow', 'blue', 1);
- sharedLabelColor.addSlice('pig', 'red', 1);
- sharedLabelColor.addSlice('horse', 'green', 1);
- scale.removeSharedLabelColorFromRange(colorMap, 'goat');
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- sharedLabelColor.clear();
+ it('handles cases where all colors are equally used and returns the current color', () => {
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ scale.getColor('cat'); // blue
+ scale.getColor('dog'); // red
+ scale.getColor('fish'); // green
+ // All colors used once, so the function should return the current color
+ expect(scale.getNextAvailableColor('red')).toBe('red');
});
- it('should remove parentForcedColors from range', () => {
- const parentForcedColors = { house: 'blue', cow: 'red' };
- const scale = new CategoricalColorScale(
- ['blue', 'red', 'green'],
- parentForcedColors,
- );
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.clear();
- const colorMap = sharedLabelColor.getColorMap();
- scale.removeSharedLabelColorFromRange(colorMap, 'pig');
- expect(scale.range()).toEqual(['green']);
- scale.removeSharedLabelColorFromRange(colorMap, 'cow');
- expect(scale.range()).toEqual(['red', 'green']);
- sharedLabelColor.clear();
+ it('returns the least used color accurately even when some colors are used more frequently', () => {
+ const scale = new CategoricalColorScale([
+ 'blue',
+ 'red',
+ 'green',
+ 'yellow',
+ ]);
+ scale.getColor('cat'); // blue
+ scale.getColor('dog'); // red
+ scale.getColor('frog'); // green
+ scale.getColor('fish'); // yellow
+ scale.getColor('goat'); // blue
+ scale.getColor('horse'); // red
+ scale.getColor('pony'); // green
+
+ // Yellow is the least used color, so it should be returned.
+ expect(scale.getNextAvailableColor('blue')).toBe('yellow');
+ });
+ });
+
+ describe("is compatible with D3's ScaleOrdinal", () => {
+ it('passes type check', () => {
+ const scale: ScaleOrdinal<{ toString(): string }, string> =
+ new CategoricalColorScale(['blue', 'red', 'green']);
+ expect(scale('pig')).toBe('blue');
});
});
});
diff --git a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
index 8f89ae8a69..ae23f34cc4 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
@@ -96,13 +96,7 @@ describe('SharedLabelColor', () => {
it('should do nothing when source is not dashboard', () => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.source = SharedLabelColorSource.Explore;
- sharedLabelColor.addSlice('a', 'red');
- expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({});
- });
-
- it('should do nothing when sliceId is undefined', () => {
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.addSlice('a', 'red');
+ sharedLabelColor.addSlice('a', 'red', 1);
expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({});
});
});
@@ -144,8 +138,8 @@ describe('SharedLabelColor', () => {
const colorMap = sharedLabelColor.getColorMap();
expect(Object.fromEntries(colorMap)).toEqual({
a: 'yellow',
- b: 'yellow',
- c: 'green',
+ b: 'green',
+ c: 'yellow',
});
});
@@ -195,9 +189,9 @@ describe('SharedLabelColor', () => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.addSlice('a', 'red', 1);
sharedLabelColor.addSlice('b', 'blue', 2);
- sharedLabelColor.reset();
+ sharedLabelColor.clear();
const colorMap = sharedLabelColor.getColorMap();
- expect(Object.fromEntries(colorMap)).toEqual({ a: '', b: '' });
+ expect(Object.fromEntries(colorMap)).toEqual({});
});
});
});
diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts
index 472f945b54..36c2f02f61 100644
--- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts
+++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts
@@ -54,16 +54,15 @@ export function dashboardInfoChanged(newInfo: { metadata: any }) {
const categoricalNamespace = CategoricalColorNamespace.getNamespace(
metadata?.color_namespace,
);
-
+ // reset forced colors
categoricalNamespace.resetColors();
- if (metadata?.shared_label_colors) {
- updateColorSchema(metadata, metadata?.shared_label_colors);
- }
+ const mergedLabelColors = {
+ ...(metadata?.label_colors || {}),
+ ...(metadata?.shared_label_colors || {}),
+ };
- if (metadata?.label_colors) {
- updateColorSchema(metadata, metadata?.label_colors);
- }
+ updateColorSchema(metadata, mergedLabelColors);
return { type: DASHBOARD_INFO_UPDATED, newInfo };
}
diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx
index 485185d607..e924d22770 100644
--- a/superset-frontend/src/dashboard/components/Header/index.jsx
+++ b/superset-frontend/src/dashboard/components/Header/index.jsx
@@ -372,13 +372,17 @@ class Header extends React.PureComponent {
? currentRefreshFrequency
: dashboardInfo.metadata?.refresh_frequency;
- const currentColorScheme =
- dashboardInfo?.metadata?.color_scheme || colorScheme;
const currentColorNamespace =
dashboardInfo?.metadata?.color_namespace || colorNamespace;
+ const currentColorScheme =
+ dashboardInfo?.metadata?.color_scheme || colorScheme;
const currentSharedLabelColors = Object.fromEntries(
getSharedLabelColor().getColorMap(),
);
+ // an empty color scheme should not bring shared label colors forward
+ const sharedLabelColors = currentColorScheme
+ ? currentSharedLabelColors
+ : {};
const data = {
certified_by: dashboardInfo.certified_by,
@@ -395,7 +399,7 @@ class Header extends React.PureComponent {
color_scheme: currentColorScheme,
positions,
refresh_frequency: refreshFrequency,
- shared_label_colors: currentSharedLabelColors,
+ shared_label_colors: sharedLabelColors,
},
};
diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
index 1b5ef5fceb..d1ebc3d84e 100644
--- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
@@ -381,16 +381,21 @@ const PropertiesModal = ({
const sharedLabelColor = getSharedLabelColor();
const categoricalNamespace =
CategoricalColorNamespace.getNamespace(colorNamespace);
+
+ // reset forced colors
categoricalNamespace.resetColors();
+
if (currentColorScheme) {
+ // reset the shared label color map based on the current color scheme
sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme);
+ // store the shared label color map and domain in the metadata
metadata.shared_label_colors = Object.fromEntries(
sharedLabelColor.getColorMap(),
);
metadata.color_scheme_domain =
categoricalSchemeRegistry.get(colorScheme)?.colors || [];
} else {
- sharedLabelColor.reset();
+ sharedLabelColor.clear();
metadata.shared_label_colors = {};
metadata.color_scheme_domain = [];
}
diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
index c9f35d3ba5..fc0bf5aabd 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
@@ -18,11 +18,19 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
-import { styled, t } from '@superset-ui/core';
+import {
+ getSharedLabelColor,
+ styled,
+ SupersetClient,
+ t,
+} from '@superset-ui/core';
import { connect } from 'react-redux';
import { LineEditableTabs } from 'src/components/Tabs';
import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils';
import { AntdModal } from 'src/components';
+import { isEqual } from 'lodash';
+import jsonStringify from 'json-stringify-pretty-compact';
+import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo';
import { Draggable } from '../dnd/DragDroppable';
import DragHandle from '../dnd/DragHandle';
import DashboardComponent from '../../containers/DashboardComponent';
@@ -256,10 +264,51 @@ export class Tabs extends React.PureComponent {
}
};
+ // the initial color map is generated on save and catches all rendered charts
+ // charts hidden in nested tabs are not rendered, thus not included
+ // must update the label color map when switching tabs to include these charts
+ updateLabelColorsMap() {
+ const currentLabelColorsMap = Object.fromEntries(
+ getSharedLabelColor().getColorMap(),
+ );
+ const { metadata } = this.props.dashboardInfo;
+ const labelColorsMap = metadata?.shared_label_colors || {};
+ // color consistency is currently only supported when a color scheme is set
+ if (
+ metadata?.color_scheme &&
+ !isEqual(labelColorsMap, currentLabelColorsMap)
+ ) {
+ const updatedMetadata = {
+ ...metadata,
+ shared_label_colors: currentLabelColorsMap,
+ };
+ SupersetClient.put({
+ endpoint: `/api/v1/dashboard/${this.props.dashboardId}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ json_metadata: jsonStringify(updatedMetadata),
+ }),
+ })
+ .then(() =>
+ this.props.dispatch(
+ dashboardInfoChanged({
+ metadata: updatedMetadata,
+ }),
+ ),
+ )
+ .catch(e => console.error(e));
+ }
+ }
+
handleClickTab(tabIndex) {
const { component } = this.props;
const { children: tabIds } = component;
+ // the charts need to render first
+ setTimeout(() => {
+ this.updateLabelColorsMap();
+ }, 500);
+
if (tabIndex !== this.state.tabIndex) {
const pathToTabIndex = getDirectPathToTabIndex(component, tabIndex);
const targetTabId = pathToTabIndex[pathToTabIndex.length - 1];
diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
index 68478adb07..f600463ee9 100644
--- a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
+++ b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
@@ -78,6 +78,7 @@ function mapStateToProps(
editMode: dashboardState.editMode,
filters: getActiveFilters(),
dashboardId: dashboardInfo.id,
+ dashboardInfo,
fullSizeChartId: dashboardState.fullSizeChartId,
};
diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
index c01dc76f1e..d1bb7c2ac4 100644
--- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx
+++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
@@ -188,6 +188,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
useEffect(() => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.source = SharedLabelColorSource.Dashboard;
+
return () => {
// clean up label color
const categoricalNamespace = CategoricalColorNamespace.getNamespace(
diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
index dba78af035..d7c1a804a9 100644
--- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
@@ -24,6 +24,7 @@ import { Tooltip } from 'src/components/Tooltip';
import {
CategoricalColorNamespace,
css,
+ getSharedLabelColor,
logging,
SupersetClient,
t,
@@ -95,6 +96,16 @@ export const ExploreChartHeader = ({
const { dashboards } = metadata || {};
const dashboard =
dashboardId && dashboards && dashboards.find(d => d.id === dashboardId);
+ const categoricalNamespace = CategoricalColorNamespace.getNamespace();
+ const sharedLabelColor = getSharedLabelColor();
+
+ if (!dashboard) {
+ // clean up color namespace and shared color maps
+ // to avoid colors spill outside of dashboard context
+ categoricalNamespace.resetColors();
+ sharedLabelColor.clear();
+ return;
+ }
if (dashboard) {
try {
@@ -106,21 +117,19 @@ export const ExploreChartHeader = ({
const result = response?.json?.result;
// setting the chart to use the dashboard custom label colors if any
- const metadata = JSON.parse(result.json_metadata);
- const sharedLabelColors = metadata.shared_label_colors || {};
- const customLabelColors = metadata.label_colors || {};
+ const dashboardMetadata = JSON.parse(result.json_metadata);
+ const sharedLabelColors = dashboardMetadata.shared_label_colors || {};
+ const customLabelColors = dashboardMetadata.label_colors || {};
const mergedLabelColors = {
...sharedLabelColors,
...customLabelColors,
};
- const categoricalNamespace = CategoricalColorNamespace.getNamespace();
-
Object.keys(mergedLabelColors).forEach(label => {
categoricalNamespace.setColor(
label,
mergedLabelColors[label],
- metadata.color_scheme,
+ dashboardMetadata.color_scheme,
);
});
} catch (error) {
@@ -130,7 +139,7 @@ export const ExploreChartHeader = ({
};
useEffect(() => {
- if (dashboardId) updateCategoricalNamespace();
+ updateCategoricalNamespace();
}, []);
const openPropertiesModal = () => {
diff --git a/tests/integration_tests/superset_test_config.py b/tests/integration_tests/superset_test_config.py
index b0461e79fc..977eb1c6b5 100644
--- a/tests/integration_tests/superset_test_config.py
+++ b/tests/integration_tests/superset_test_config.py
@@ -69,6 +69,7 @@ FEATURE_FLAGS = {
"SHARE_QUERIES_VIA_KV_STORE": True,
"ENABLE_TEMPLATE_PROCESSING": True,
"ALERT_REPORTS": True,
+ "AVOID_COLORS_COLLISION": True,
"DRILL_TO_DETAIL": True,
"DRILL_BY": True,
"HORIZONTAL_FILTER_BAR": True,
(superset) 02/04: Prefer less used colors
Posted by di...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 43fe16227dbd760c295a2e337de6f077d921cd8a
Author: geido <di...@gmail.com>
AuthorDate: Fri Mar 1 20:54:29 2024 +0200
Prefer less used colors
---
.../src/color/CategoricalColorScale.ts | 185 +++++++++++----------
.../packages/superset-ui-core/src/color/utils.ts | 5 +-
2 files changed, 102 insertions(+), 88 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
index 8b8cd02519..c4e96a9a1f 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
@@ -1,23 +1,3 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-/* eslint-disable no-dupe-class-members */
import { scaleOrdinal, ScaleOrdinal } from 'd3-scale';
import { ExtensibleFunction } from '../models';
import { ColorsInitLookup, ColorsLookup } from './types';
@@ -39,105 +19,143 @@ class CategoricalColorScale extends ExtensibleFunction {
scale: ScaleOrdinal<{ toString(): string }, string>;
- parentForcedColors: ColorsLookup;
-
forcedColors: ColorsLookup;
+ colorUsageCount: Map<string, number>;
+
+ 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));
-
this.originColors = colors;
this.colors = colors;
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 = {};
+ // forced colors from parent (usually CategoricalColorNamespace)
+ this.forcedColors = forcedColors as ColorsLookup;
+ // holds the usage count for each color including forced colors from parent
+ this.colorUsageCount = this.initColorUsageCounter();
+ // holds the values in this specific slice
+ this.sliceMap = new Map();
+ // holds the multiple value for analogous colors range
this.multiple = 0;
}
- 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);
- }
+ /**
+ * 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);
+ this.range(this.originColors.concat(newRange));
+ // update the colorUsageCount map with the new colors
+ newRange.forEach(color => {
+ this.colorUsageCount.set(color, 0);
+ });
+ }
+ }
+
+ /**
+ * Initializes the color usage count map
+ * @returns a map of color to usage count
+ */
+ initColorUsageCounter(): Map<string, number> {
+ const colorUsageCount = new Map(this.colors.map(color => [color, 0]));
+ // add forced colors to the usage count map
+ // helps reducing conflicts but increases randomness
+ Object.values(this.forcedColors).forEach(color => {
+ const count = colorUsageCount.get(color) || 0;
+ colorUsageCount.set(color, count + 1);
});
- this.range(updatedRange.size > 0 ? [...updatedRange] : this.originColors);
+ return colorUsageCount;
}
- getColor(value?: string, sliceId?: number) {
+ getColor(value?: string, sliceId?: number): string {
const cleanedValue = stringifyAndTrim(value);
const sharedLabelColor = getSharedLabelColor();
const sharedColorMap = sharedLabelColor.getColorMap();
const sharedColor = sharedColorMap.get(cleanedValue);
+ const forcedColor = this.forcedColors?.[cleanedValue] || sharedColor;
+ let color = forcedColor || this.scale(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));
+ // a forced color will always be used independently of the usage count
+ if (!forcedColor) {
+ 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 (this.isColorUsed(color)) {
+ // color was used in this slice already
+ // fallback to least used color
+ color = this.getNextAvailableColor(color);
}
}
+ // increment the usage count for the color
+ this.incrementColorUsage(color);
+
+ // store the value+color in the shared map for dashboard consistency
sharedLabelColor.addSlice(cleanedValue, color, sliceId);
+ // keep track of values in this slice
+ this.sliceMap.set(cleanedValue, color);
+
return color;
}
/**
- * Enforce specific color for given value
- * @param {*} value value
- * @param {*} forcedColor forcedColor
+ *
+ * @param color
+ * @returns whether the color is used in this slice
*/
- setColor(value: string, forcedColor: string) {
- this.forcedColors[stringifyAndTrim(value)] = forcedColor;
- return this;
+ isColorUsed(color: string): boolean {
+ return Array.from(this.sliceMap.values()).includes(color);
+ }
+
+ /**
+ * Lower chances of color collision by returning the least used color
+ * Checks across colors of current slice and forced colors (all slices from parent)
+ *
+ * @param excludeColor
+ * @returns the least used color that is not the excluded color
+ */
+ getNextAvailableColor(excludeColor: string) {
+ // sort the colors by count, then exclude the specified color
+ const sortedColors = [...this.colorUsageCount.entries()]
+ .sort((a, b) => a[1] - b[1])
+ // eslint-disable-next-line @typescript-eslint/no-unused-vars
+ .filter(([color, _]) => color !== excludeColor);
+ const color = sortedColors[0][0];
+
+ return color;
+ }
+
+ incrementColorUsage(color: string) {
+ const currentCount = this.colorUsageCount.get(color) || 0;
+ this.colorUsageCount.set(color, currentCount + 1);
}
/**
@@ -145,16 +163,11 @@ class CategoricalColorScale extends ExtensibleFunction {
* @returns an object where the key is the data value and the value is the hex color code
*/
getColorMap() {
- const colorMap: { [key: string]: string | undefined } = {};
+ const colorMap = {};
this.scale.domain().forEach(value => {
colorMap[value.toString()] = this.scale(value);
});
-
- return {
- ...colorMap,
- ...this.forcedColors,
- ...this.parentForcedColors,
- };
+ return { ...colorMap, ...this.forcedColors };
}
/**
@@ -163,12 +176,12 @@ class CategoricalColorScale extends ExtensibleFunction {
copy() {
const copy = new CategoricalColorScale(
this.scale.range(),
- this.parentForcedColors,
+ this.forcedColors,
);
copy.forcedColors = { ...this.forcedColors };
+ copy.colorUsageCount = new Map(this.colorUsageCount);
copy.domain(this.domain());
copy.unknown(this.unknown());
-
return copy;
}
diff --git a/superset-frontend/packages/superset-ui-core/src/color/utils.ts b/superset-frontend/packages/superset-ui-core/src/color/utils.ts
index 55284f16b4..c0f33f9f8c 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/utils.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/utils.ts
@@ -55,11 +55,12 @@ export function getContrastingColor(color: string, thresholds = 186) {
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;
+
const analogousColors = colors.map(color => {
+ // returns an array of tinycolor instances
const result = tinycolor(color).analogous(results + ext);
+ // remove the first three colors to avoid the same or very close colors
return result.slice(ext);
});
(superset) 01/04: Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
Posted by di...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 91f0d51abd0d60dca5ccd72e14d144614f46abf5
Merge: 127df24c08 ad6327db95
Author: geido <di...@gmail.com>
AuthorDate: Fri Mar 1 17:21:20 2024 +0200
Merge branch 'master' of https://github.com/apache/superset into diego/ch79154/fix-color-inconsistency
.asf.yaml | 8 -
.github/workflows/chromatic-master.yml | 72 -
.github/workflows/docker.yml | 42 +-
.github/workflows/no-op.yml | 2 +-
.github/workflows/superset-python-unittest.yml | 2 +
RELEASING/validate_this_release.sh | 7 +-
docs/docs/installation/cache.mdx | 2 +-
docs/docs/installation/docker.mdx | 66 +-
docs/docs/installation/sql-templating.mdx | 23 +-
docs/docs/security/cves.mdx | 10 +
docs/package.json | 2 +-
docs/static/.htaccess | 2 +
docs/yarn.lock | 128 +-
requirements/base.in | 2 +
requirements/base.txt | 21 +-
requirements/development.txt | 4 +-
requirements/docker.in | 3 +-
requirements/docker.txt | 4 +-
requirements/integration.txt | 6 +
requirements/testing.in | 1 +
requirements/testing.txt | 7 +-
scripts/ci_check_no_file_changes.sh | 2 +-
scripts/tag_latest_release.sh | 12 +-
setup.py | 10 +-
superset-embedded-sdk/README.md | 7 +-
superset-embedded-sdk/package-lock.json | 4 +-
superset-embedded-sdk/package.json | 2 +-
superset-embedded-sdk/src/index.ts | 18 +-
.../cypress/e2e/chart_list/list.test.ts | 7 +
.../cypress/e2e/dashboard/editmode.test.ts | 2 +-
.../cypress/e2e/dashboard_list/list.test.ts | 7 +
superset-frontend/package-lock.json | 2656 +-------------------
superset-frontend/package.json | 5 +-
.../packages/superset-ui-demo/package.json | 1 -
.../shared/components/createQuery.story.tsx | 3 -
.../superset-ui-connection/Connection.stories.tsx | 12 +-
.../BigNumberPeriodOverPeriod}/PopKPI.tsx | 0
.../BigNumberPeriodOverPeriod}/buildQuery.ts | 15 -
.../BigNumberPeriodOverPeriod}/controlPanel.ts | 67 +-
.../images/thumbnail.png | Bin
.../BigNumber/BigNumberPeriodOverPeriod}/index.ts | 15 +-
.../BigNumberPeriodOverPeriod}/transformProps.ts | 7 +-
.../BigNumber/BigNumberPeriodOverPeriod}/types.ts | 0
.../BigNumberPeriodOverPeriod/utils.test.ts | 39 +
.../BigNumber/BigNumberPeriodOverPeriod/utils.ts | 59 +
.../plugin-chart-echarts/src/BigNumber/index.ts | 1 +
.../src/Gauge/transformProps.ts | 13 +-
.../src/Timeseries/transformProps.ts | 2 +-
.../src/Timeseries/transformers.ts | 7 +-
.../plugins/plugin-chart-echarts/src/index.ts | 6 +-
.../test/Gauge/transformProps.test.ts | 54 +-
.../plugin-chart-period-over-period-kpi/README.md | 87 -
.../package.json | 33 -
.../src/index.ts | 27 -
.../tsconfig.json | 25 -
.../types/types/external.d.ts | 23 -
.../SqlLab/components/SqlEditor/SqlEditor.test.tsx | 43 +-
.../src/SqlLab/components/SqlEditor/index.tsx | 28 +-
.../src/SqlLab/reducers/getInitialState.ts | 2 +-
superset-frontend/src/SqlLab/reducers/sqlLab.js | 5 +-
.../src/SqlLab/reducers/sqlLab.test.js | 19 +
.../src/components/ListView/CardSortSelect.tsx | 18 +-
.../src/components/ListView/ListView.test.tsx | 74 +
.../src/components/ListView/ListView.tsx | 17 +-
superset-frontend/src/components/ListView/types.ts | 4 +-
superset-frontend/src/components/ListView/utils.ts | 4 +-
.../src/dashboard/components/dnd/DragDroppable.jsx | 6 +
.../filters/components/Range/RangeFilterPlugin.tsx | 14 +
superset-frontend/src/types/dom-to-pdf.d.ts | 6 +-
.../src/visualizations/presets/MainPreset.js | 8 +-
superset/charts/data/api.py | 1 +
superset/cli/importexport.py | 39 +-
superset/db_engine_specs/base.py | 2 +-
superset/jinja_context.py | 36 +-
...47_be1b217cd8cd_big_number_kpi_single_metric.py | 93 +
superset/sql_parse.py | 17 +-
superset/utils/core.py | 13 +
superset/views/database/forms.py | 3 +-
tests/integration_tests/charts/data/api_tests.py | 36 +-
tests/integration_tests/sqla_models_tests.py | 28 +-
tests/unit_tests/db_engine_specs/test_base.py | 75 +-
.../unit_tests/scripts/tag_latest_release_test.py | 4 +-
tests/unit_tests/sql_parse_tests.py | 10 +-
tests/unit_tests/test_jinja_context.py | 38 +
84 files changed, 1016 insertions(+), 3269 deletions(-)