Skip to content

Commit

Permalink
fix: repeated color in the same chart (#23762)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lilykuang
lilykuang committed May 19, 2023
1 parent 6159ced commit 66594ad
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 19 deletions.
34 changes: 17 additions & 17 deletions superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ function selectColorScheme(color: string) {
}

function applyChanges() {
cy.getBySel('properties-modal-apply-button').click();
cy.getBySel('properties-modal-apply-button').click({ force: true });
}

function saveChanges() {
Expand Down Expand Up @@ -209,7 +209,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
});

it('should apply same color to same labels with color scheme set', () => {
Expand All @@ -230,7 +230,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(234, 11, 140)');
.should('have.css', 'fill', 'rgb(51, 217, 193)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -239,7 +239,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(234, 11, 140)');
.should('have.css', 'fill', 'rgb(51, 217, 193)');
});

it('should apply same color to same labels with no color scheme set', () => {
Expand All @@ -260,7 +260,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -269,7 +269,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
});

it('custom label colors should take the precedence in nested tabs', () => {
Expand Down Expand Up @@ -383,17 +383,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(224, 67, 85)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
.should('have.css', 'fill', 'rgb(90, 193, 137)');
.should('have.css', 'fill', 'rgb(168, 104, 183)');

openProperties();
cy.get('[aria-label="Select color scheme"]').should('have.value', '');
Expand Down Expand Up @@ -422,17 +422,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(224, 67, 85)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
.should('have.css', 'fill', 'rgb(90, 193, 137)');
.should('have.css', 'fill', 'rgb(168, 104, 183)');
});

it('should show the same colors in Explore', () => {
Expand Down Expand Up @@ -463,7 +463,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(108, 131, 142)');
.should('have.css', 'fill', 'rgb(157, 172, 185)');

openExplore('Top 10 California Names Timeseries');

Expand Down Expand Up @@ -495,7 +495,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(234, 11, 140)');
.should('have.css', 'fill', 'rgb(51, 217, 193)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -504,7 +504,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(234, 11, 140)');
.should('have.css', 'fill', 'rgb(51, 217, 193)');

editDashboard();
openProperties();
Expand Down Expand Up @@ -569,7 +569,7 @@ describe('Dashboard edit', () => {

cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(255, 90, 95)');
.should('have.css', 'fill', 'rgb(140, 224, 113)');

// go back to first tab
openTab(0, 0);
Expand Down Expand Up @@ -616,7 +616,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(234, 11, 140)');
.should('have.css', 'fill', 'rgb(51, 217, 193)');

// open another nested tab
openTab(2, 1);
Expand Down
25 changes: 24 additions & 1 deletion superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,33 @@ class CategoricalColorScale extends ExtensibleFunction {
this.multiple = 0;
}

removeSharedLabelColorFromRange(
sharedColorMap: Map<string, string>,
cleanedValue: string,
) {
// make sure we don't overwrite the origin colors
const updatedRange = [...this.originColors];
// remove the color option from shared color
sharedColorMap.forEach((value: string, key: string) => {
if (key !== cleanedValue) {
const index = updatedRange.indexOf(value);
updatedRange.splice(index, 1);
}
});
this.range(updatedRange.length > 0 ? updatedRange : this.originColors);
}

getColor(value?: string, sliceId?: number) {
const cleanedValue = stringifyAndTrim(value);
const sharedLabelColor = getSharedLabelColor();
const sharedColorMap = sharedLabelColor.getColorMap();
const sharedColor = sharedColorMap.get(cleanedValue);

// priority: parentForcedColors > forcedColors > labelColors
let color =
this.parentForcedColors?.[cleanedValue] ||
this.forcedColors?.[cleanedValue] ||
sharedLabelColor.getColorMap().get(cleanedValue);
sharedColor;

if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) {
const multiple = Math.floor(
Expand All @@ -96,7 +114,12 @@ class CategoricalColorScale extends ExtensibleFunction {
const newColor = this.scale(cleanedValue);
if (!color) {
color = newColor;
if (isFeatureEnabled(FeatureFlag.AVOID_COLORS_COLLISION)) {
this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue);
color = this.scale(cleanedValue);
}
}

sharedLabelColor.addSlice(cleanedValue, color, sliceId);

return color;
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export enum FeatureFlag {
TAGGING_SYSTEM = 'TAGGING_SYSTEM',
VERSIONED_EXPORT = 'VERSIONED_EXPORT',
SSH_TUNNELING = 'SSH_TUNNELING',
AVOID_COLORS_COLLISION = 'AVOID_COLORS_COLLISION',
}
export type ScheduleQueriesProps = {
JSONSCHEMA: {
Expand Down
54 changes: 53 additions & 1 deletion superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
*/

import { ScaleOrdinal } from 'd3-scale';
import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core';
import {
CategoricalColorScale,
FeatureFlag,
getSharedLabelColor,
} from '@superset-ui/core';

describe('CategoricalColorScale', () => {
it('exists', () => {
Expand Down Expand Up @@ -116,6 +120,24 @@ describe('CategoricalColorScale', () => {
scale.getColor('goat');
expect(scale.range()).toHaveLength(6);
});

it('should remove shared color from range if avoid colors collision enabled', () => {
window.featureFlags = {
[FeatureFlag.AVOID_COLORS_COLLISION]: 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.AVOID_COLORS_COLLISION]: false,
};
});
describe('.setColor(value, forcedColor)', () => {
it('overrides default color', () => {
Expand Down Expand Up @@ -222,4 +244,34 @@ describe('CategoricalColorScale', () => {
expect(scale('pig')).toBe('blue');
});
});

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']);

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();
});

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();
});
});
});
1 change: 1 addition & 0 deletions superset/config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ class D3Format(TypedDict, total=False):
# Users must check whether the DB engine supports SSH Tunnels
# otherwise enabling this flag won't have any effect on the DB.
"SSH_TUNNELING": False,
"AVOID_COLORS_COLLISION": True,
}

# ------------------------------
Expand Down

0 comments on commit 66594ad

Please sign in to comment.