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 b7ea17f79f6ec..0f7005bf85470 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -16,10 +16,19 @@ * specific language governing permissions and limitations * under the License. */ -import { SAMPLE_DASHBOARD_1, TABBED_DASHBOARD } from 'cypress/utils/urls'; +import { + SAMPLE_DASHBOARD_1, + SUPPORTED_CHARTS_DASHBOARD, + 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 { + interceptExploreUpdate, + interceptGet, + interceptUpdate, + openTab, +} from './utils'; import { interceptExploreJson, interceptFiltering as interceptCharts, @@ -42,15 +51,37 @@ function openProperties() { cy.getBySel('header-actions-menu') .contains('Edit properties') .click({ force: true }); - cy.wait(500); + cy.get('.ant-modal-body').should('be.visible'); }); } +function openExploreProperties() { + cy.getBySel('actions-trigger').click({ force: true }); + cy.get('.ant-dropdown-menu') + .contains('Edit chart properties') + .click({ force: true }); + cy.get('.ant-modal-body').should('be.visible'); +} + +function assertMetadata(text: string) { + const regex = new RegExp(text); + cy.get('#json_metadata') + .should('be.visible') + .then(() => { + const metadata = cy.$$('#json_metadata')[0]; + + // cypress can read this locally, but not in ci + // so we have to use the ace module directly to fetch the value + expect(ace.edit(metadata).getValue()).to.match(regex); + }); +} + function openAdvancedProperties() { cy.get('.ant-modal-body') .contains('Advanced') .should('be.visible') .click({ force: true }); + cy.get('#json_metadata').should('be.visible'); } function dragComponent( @@ -83,20 +114,36 @@ function visitEdit(sampleDashboard = SAMPLE_DASHBOARD_1) { cy.visit(sampleDashboard); cy.wait('@get'); editDashboard(); + cy.get('.grid-container').should('exist'); cy.wait('@filtering'); cy.wait(500); } -function resetTabbedDashboard(go = false) { +function visit(sampleDashboard = SAMPLE_DASHBOARD_1) { + interceptCharts(); + interceptGet(); + + if (sampleDashboard === SAMPLE_DASHBOARD_1) { + cy.createSampleDashboards([0]); + } + + cy.visit(sampleDashboard); + cy.wait('@get'); + cy.get('.grid-container').should('exist'); + cy.wait(500); +} + +function resetDashboardColors(dashboard = 'tabbed_dash') { // eslint-disable-next-line @typescript-eslint/no-explicit-any - cy.getDashboard('tabbed_dash').then((r: Record) => { + cy.getDashboard(dashboard).then((r: Record) => { const jsonMetadata = r?.json_metadata || '{}'; const metadata = JSON.parse(jsonMetadata); const resetMetadata = JSON.stringify({ ...metadata, color_scheme: '', label_colors: {}, - shared_label_colors: {}, + shared_label_colors: [], + map_label_colors: {}, }); cy.updateDashboard(r.id, { certification_details: r.certification_details, @@ -106,27 +153,37 @@ function resetTabbedDashboard(go = false) { json_metadata: resetMetadata, owners: r.owners, slug: r.slug, - }).then(() => { - if (go) { - visitEdit(TABBED_DASHBOARD); - } }); }); } -function visitResetTabbedDashboard() { - resetTabbedDashboard(true); -} - -function selectColorScheme(color: string) { - cy.get( - '[data-test="dashboard-edit-properties-form"] [aria-label="Select color scheme"]', - ) +function selectColorScheme( + color: string, + target = 'dashboard-edit-properties-form', +) { + cy.get(`[data-test="${target}"] input[aria-label="Select color scheme"]`) .first() - .click(); + .then($input => { + cy.wrap($input).click({ force: true }); + cy.wrap($input).type(color.slice(0, 5), { force: true }); + }); cy.getBySel(color).click({ force: true }); } +function saveAndGo(dashboard = 'Tabbed Dashboard') { + interceptExploreUpdate(); + cy.getBySel('query-save-button').click(); + cy.getBySel('save-modal-body').then($modal => { + cy.wrap($modal) + .find("div[aria-label='Select a dashboard'] .ant-select-selection-item") + .should('have.text', dashboard); + cy.getBySel('save-overwrite-radio').should('not.be.disabled'); + cy.getBySel('save-overwrite-radio').click(); + cy.get('#btn_modal_save_goto_dash').click(); + cy.wait('@chartUpdate'); + }); +} + function applyChanges() { cy.getBySel('properties-modal-apply-button').click({ force: true }); } @@ -137,37 +194,37 @@ function saveChanges() { cy.wait('@update'); } -function assertMetadata(text: string) { - const regex = new RegExp(text); - cy.get('#json_metadata') - .should('be.visible') - .then(() => { - const metadata = cy.$$('#json_metadata')[0]; - - // cypress can read this locally, but not in ci - // so we have to use the ace module directly to fetch the value - expect(ace.edit(metadata).getValue()).to.match(regex); - }); -} function clearMetadata() { cy.get('#json_metadata').then($jsonmetadata => { - cy.wrap($jsonmetadata).find('.ace_content').click(); + cy.wrap($jsonmetadata).find('.ace_content').click({ force: true }); cy.wrap($jsonmetadata) .find('.ace_text-input') - .type('{selectall} {backspace}', { force: true }); + .then($ace => { + cy.wrap($ace).focus(); + cy.wrap($ace).should('have.focus'); + cy.wrap($ace).type('{selectall}', { force: true }); + cy.wrap($ace).type('{backspace}', { force: true }); + }); }); } function writeMetadata(metadata: string) { - cy.get('#json_metadata').then($jsonmetadata => - cy - .wrap($jsonmetadata) + cy.get('#json_metadata').then($jsonmetadata => { + cy.wrap($jsonmetadata).find('.ace_content').click({ force: true }); + cy.wrap($jsonmetadata) .find('.ace_text-input') - .type(metadata, { parseSpecialCharSequences: false, force: true }), - ); + .then($ace => { + cy.wrap($ace).focus(); + cy.wrap($ace).should('have.focus'); + cy.wrap($ace).type(metadata, { + parseSpecialCharSequences: false, + force: true, + }); + }); + }); } -function openExplore(chartName: string) { +function openExploreWithDashboardContext(chartName: string) { interceptExploreJson(); interceptGet(); @@ -181,21 +238,91 @@ function openExplore(chartName: string) { .should('contain', 'Edit chart') .click(); cy.wait('@getJson'); + cy.get('.chart-container').should('exist'); +} + +function saveExploreColorScheme( + chart = 'Top 10 California Names Timeseries', + colorScheme = 'supersetColors', +) { + interceptExploreUpdate(); + openExploreWithDashboardContext(chart); + openTab(0, 1, 'control-tabs'); + selectColorScheme(colorScheme, 'control-item'); + cy.getBySel('query-save-button').click(); + cy.getBySel('save-overwrite-radio').click(); + cy.getBySel('btn-modal-save').click(); + cy.wait('@chartUpdate'); } describe('Dashboard edit', () => { describe('Color consistency', () => { beforeEach(() => { - visitResetTabbedDashboard(); + resetDashboardColors(); }); - after(() => { - resetTabbedDashboard(); + it('should not allow to change color scheme of a chart when dashboard has one', () => { + visitEdit(TABBED_DASHBOARD); + openProperties(); + selectColorScheme('blueToGreen'); + applyChanges(); + saveChanges(); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + openExploreWithDashboardContext('Top 10 California Names Timeseries'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + openTab(0, 1, 'control-tabs'); + + cy.get('[aria-label="Select color scheme"]').should('be.disabled'); }); - it('should respect chart color scheme when none is set for the dashboard', () => { + it('should not allow to change color scheme of a chart when dashboard has no scheme but chart has shared labels', () => { + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // open second top tab to catch shared labels + openTab(0, 1); + waitForChartLoad({ + name: 'Trends', + viz: 'line', + }); + + openTab(0, 0); + openExploreWithDashboardContext('Top 10 California Names Timeseries'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + openTab(0, 1, 'control-tabs'); + + cy.get('[aria-label="Select color scheme"]').should('be.disabled'); + }); + + it('should allow to change color scheme of a chart when dashboard has no scheme but only custom label colors', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); - cy.get('[aria-label="Select color scheme"]').should('have.value', ''); + openAdvancedProperties(); + clearMetadata(); + writeMetadata('{"color_scheme":"","label_colors":{"Anthony":"red"}}'); applyChanges(); saveChanges(); @@ -206,17 +333,93 @@ describe('Dashboard edit', () => { viz: 'line', }); + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(255, 0, 0)'); + + openExploreWithDashboardContext('Top 10 California Names Timeseries'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(255, 0, 0)'); + + openTab(0, 1, 'control-tabs'); + selectColorScheme('blueToGreen', 'control-item'); + + // label Anthony + 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(50, 0, 167)'); + + // label Daniel + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .eq(2) + .should('have.css', 'fill', 'rgb(0, 76, 218)'); + + // label David + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .eq(3) + .should('have.css', 'fill', 'rgb(0, 116, 241)'); + }); + + it('should allow to change color scheme of a chart when dashboard has no scheme and show the change', () => { + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + // label Anthony cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + openExploreWithDashboardContext('Top 10 California Names Timeseries'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + openTab(0, 1, 'control-tabs'); + selectColorScheme('blueToGreen', 'control-item'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + saveAndGo(); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + // reset original scheme + saveExploreColorScheme(); }); - it('should apply same color to same labels with color scheme set', () => { + it('should allow to change color scheme of a chart when dashboard has no scheme but custom label colors and show the change', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); - selectColorScheme('blueToGreen'); + openAdvancedProperties(); + clearMetadata(); + writeMetadata('{"color_scheme":"","label_colors":{"Anthony":"red"}}'); applyChanges(); saveChanges(); @@ -232,21 +435,174 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() + .should('have.css', 'fill', 'rgb(255, 0, 0)'); + + openExploreWithDashboardContext('Top 10 California Names Timeseries'); + + // label Anthony + cy.get('[data-test="chart-container"] .line .nv-legend-symbol') + .first() + .should('have.css', 'fill', 'rgb(255, 0, 0)'); + + openTab(0, 1, 'control-tabs'); + selectColorScheme('blueToGreen', 'control-item'); + + // label Anthony + 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(50, 0, 167)'); + + saveAndGo(); + + // label Anthony + 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(50, 0, 167)'); + // reset original scheme + saveExploreColorScheme(); + }); + + it('should not change colors on refreshes with no color scheme set', () => { + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + // open 2nd main tab openTab(0, 1); waitForChartLoad({ name: 'Trends', viz: 'line' }); + // label Andrew + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(1) + .should('have.css', 'fill', 'rgb(69, 78, 124)'); + + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Andrew cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') - .eq(2) + .eq(1) + .should('have.css', 'fill', 'rgb(69, 78, 124)'); + }); + + it('should not change colors on refreshes with color scheme set', () => { + visitEdit(TABBED_DASHBOARD); + openProperties(); + selectColorScheme('blueToGreen'); + applyChanges(); + saveChanges(); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Andrew + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(1) + .should('have.css', 'fill', 'rgb(0, 76, 218)'); + + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Andrew + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(1) + .should('have.css', 'fill', 'rgb(0, 76, 218)'); + }); + + it('should respect chart color scheme when none is set for the dashboard', () => { + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); }); - it('should apply same color to same labels with no color scheme set', () => { + it('should apply same color to same labels with color scheme set on refresh', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); - cy.get('[aria-label="Select color scheme"]').should('have.value', ''); + selectColorScheme('blueToGreen'); applyChanges(); saveChanges(); @@ -257,6 +613,82 @@ describe('Dashboard edit', () => { viz: 'line', }); + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Anthony + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(2) + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + visit(TABBED_DASHBOARD); + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Anthony + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(2) + .should('have.css', 'fill', 'rgb(50, 0, 167)'); + }); + + it('should apply same color to same labels with no color scheme set on refresh', () => { + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + + // label Anthony + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', + ) + .first() + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + // open 2nd main tab + openTab(0, 1); + waitForChartLoad({ name: 'Trends', viz: 'line' }); + + // label Anthony + cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') + .eq(2) + .should('have.css', 'fill', 'rgb(31, 168, 201)'); + + visit(TABBED_DASHBOARD); + + // open nested tab + openTab(1, 1); + waitForChartLoad({ + name: 'Top 10 California Names Timeseries', + viz: 'line', + }); + // label Anthony cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', @@ -275,6 +707,7 @@ describe('Dashboard edit', () => { }); it('custom label colors should take the precedence in nested tabs', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); openAdvancedProperties(); clearMetadata(); @@ -305,6 +738,7 @@ describe('Dashboard edit', () => { }); it('label colors should take the precedence for rendered charts in nested tabs', () => { + visitEdit(TABBED_DASHBOARD); // open the tab first time and let chart load openTab(1, 1); waitForChartLoad({ @@ -333,6 +767,7 @@ describe('Dashboard edit', () => { }); it('should re-apply original color after removing custom label color with color scheme set', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); openAdvancedProperties(); clearMetadata(); @@ -375,6 +810,7 @@ describe('Dashboard edit', () => { }); it('should re-apply original color after removing custom label color with no color scheme set', () => { + visitEdit(TABBED_DASHBOARD); // open nested tab openTab(1, 1); waitForChartLoad({ @@ -438,6 +874,7 @@ describe('Dashboard edit', () => { }); it('should show the same colors in Explore', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); openAdvancedProperties(); clearMetadata(); @@ -461,7 +898,7 @@ describe('Dashboard edit', () => { .first() .should('have.css', 'fill', 'rgb(255, 0, 0)'); - openExplore('Top 10 California Names Timeseries'); + openExploreWithDashboardContext('Top 10 California Names Timeseries'); // label Anthony cy.get('[data-test="chart-container"] .line .nv-legend-symbol') @@ -469,7 +906,8 @@ describe('Dashboard edit', () => { .should('have.css', 'fill', 'rgb(255, 0, 0)'); }); - it.skip('should change color scheme multiple times', () => { + it('should change color scheme multiple times', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); selectColorScheme('blueToGreen'); applyChanges(); @@ -487,7 +925,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(50, 0, 167)'); // open 2nd main tab openTab(0, 1); @@ -496,7 +934,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(50, 0, 167)'); editDashboard(); openProperties(); @@ -507,7 +945,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(41, 105, 107)'); + .should('have.css', 'fill', 'rgb(0, 128, 246)'); // open main tab and nested tab openTab(0, 0); @@ -518,10 +956,11 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(41, 105, 107)'); + .should('have.css', 'fill', 'rgb(0, 128, 246)'); }); - it.skip('should apply the color scheme across main tabs', () => { + it('should apply the color scheme across main tabs', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); selectColorScheme('blueToGreen'); applyChanges(); @@ -533,10 +972,11 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(50, 0, 167)'); }); - it.skip('should apply the color scheme across main tabs for rendered charts', () => { + it('should apply the color scheme across main tabs for rendered charts', () => { + visitEdit(TABBED_DASHBOARD); waitForChartLoad({ name: 'Treemap', viz: 'treemap_v2' }); openProperties(); selectColorScheme('blueToGreen'); @@ -549,7 +989,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(41, 105, 107)'); + .should('have.css', 'fill', 'rgb(50, 0, 167)'); // change scheme now that charts are rendered across the main tabs editDashboard(); @@ -560,10 +1000,11 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(0, 128, 246)'); }); - it.skip('should apply the color scheme in nested tabs', () => { + it('should apply the color scheme in nested tabs', () => { + visitEdit(TABBED_DASHBOARD); openProperties(); selectColorScheme('blueToGreen'); applyChanges(); @@ -579,17 +1020,18 @@ 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(50, 0, 167)'); // open another nested tab openTab(2, 1); waitForChartLoad({ name: 'Growth Rate', viz: 'line' }); cy.get('[data-test-chart-name="Growth Rate"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(50, 0, 167)'); }); - it.skip('should apply a valid color scheme for rendered charts in nested tabs', () => { + it('should apply a valid color scheme for rendered charts in nested tabs', () => { + visitEdit(TABBED_DASHBOARD); // open the tab first time and let chart load openTab(1, 1); waitForChartLoad({ @@ -611,7 +1053,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(50, 0, 167)'); }); }); @@ -623,9 +1065,10 @@ describe('Dashboard edit', () => { beforeEach(() => { cy.createSampleDashboards([0]); openProperties(); + selectColorScheme('supersetColors'); }); - it.skip('should accept a valid color scheme', () => { + it('should accept a valid color scheme', () => { openAdvancedProperties(); clearMetadata(); writeMetadata('{"color_scheme":"lyftColors"}'); @@ -636,21 +1079,21 @@ describe('Dashboard edit', () => { applyChanges(); }); - it.skip('should overwrite the color scheme when advanced is closed', () => { + it('should overwrite the color scheme when advanced is closed', () => { selectColorScheme('blueToGreen'); openAdvancedProperties(); assertMetadata('blueToGreen'); applyChanges(); }); - it.skip('should overwrite the color scheme when advanced is open', () => { + it('should overwrite the color scheme when advanced is open', () => { openAdvancedProperties(); selectColorScheme('modernSunset'); assertMetadata('modernSunset'); applyChanges(); }); - it.skip('should not accept an invalid color scheme', () => { + it('should not accept an invalid color scheme', () => { openAdvancedProperties(); clearMetadata(); // allow console error @@ -716,7 +1159,7 @@ describe('Dashboard edit', () => { visitEdit(); }); - it.skip('should add charts', () => { + it('should add charts', () => { cy.get('[role="checkbox"]').click(); dragComponent(); cy.getBySel('dashboard-component-chart-holder').should('have.length', 1); @@ -765,7 +1208,7 @@ describe('Dashboard edit', () => { visitEdit(); }); - it.skip('should save', () => { + it('should save', () => { cy.get('[role="checkbox"]').click(); dragComponent(); cy.getBySel('header-save-button').should('be.enabled'); diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/shared_dashboard_functions.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/shared_dashboard_functions.ts index ad47116d34546..b0f7853e94ba8 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/shared_dashboard_functions.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/shared_dashboard_functions.ts @@ -91,7 +91,7 @@ export function prepareDashboardFilters( refresh_frequency: 0, color_scheme: '', label_colors: {}, - shared_label_colors: {}, + shared_label_colors: [], color_scheme_domain: [], cross_filters_enabled: false, positions: { diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts index 854f0a5880f3a..1fef884172587 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts @@ -125,7 +125,7 @@ export const valueNativeFilterOptions = [ ]; export function interceptGet() { - cy.intercept('/api/v1/dashboard/*').as('get'); + cy.intercept('GET', '/api/v1/dashboard/*').as('get'); } export function interceptFiltering() { @@ -144,6 +144,10 @@ export function interceptUpdate() { cy.intercept('PUT', `/api/v1/dashboard/*`).as('update'); } +export function interceptExploreUpdate() { + cy.intercept('PUT', `/api/v1/chart/*`).as('chartUpdate'); +} + export function interceptPost() { cy.intercept('POST', `/api/v1/dashboard/`).as('post'); } @@ -524,13 +528,17 @@ export function addCountryNameFilter() { ); } -export function openTab(tabComponentIndex: number, tabIndex: number) { - return cy - .getBySel('dashboard-component-tabs') +export function openTab( + tabComponentIndex: number, + tabIndex: number, + target = 'dashboard-component-tabs', +) { + cy.getBySel(target) .eq(tabComponentIndex) .find('[role="tab"]') .eq(tabIndex) .click(); + cy.wait(500); } export const openTopLevelTab = (tabName: string) => { diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx index 1ec3735285484..5baa8c688c9c8 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx @@ -50,6 +50,7 @@ import { import { formatSelectOptions, displayTimeRelatedControls, + getColorControlsProps, D3_FORMAT_OPTIONS, D3_FORMAT_DOCS, D3_TIME_FORMAT_OPTIONS, @@ -142,9 +143,7 @@ const linear_color_scheme: SharedControlConfig<'ColorSchemeControl'> = { renderTrigger: true, schemes: () => sequentialSchemeRegistry.getMap(), isLinear: true, - mapStateToProps: state => ({ - dashboardId: state?.form_data?.dashboardId, - }), + mapStateToProps: state => getColorControlsProps(state), }; const granularity: SharedControlConfig<'SelectControl'> = { @@ -333,9 +332,7 @@ const color_scheme: SharedControlConfig<'ColorSchemeControl'> = { choices: () => categoricalSchemeRegistry.keys().map(s => [s, s]), description: t('The color scheme for rendering chart'), schemes: () => categoricalSchemeRegistry.getMap(), - mapStateToProps: state => ({ - dashboardId: state?.form_data?.dashboardId, - }), + mapStateToProps: state => getColorControlsProps(state), }; const time_shift_color: SharedControlConfig<'CheckboxControl'> = { diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 86e7b1c04e518..9370c8d1dc80a 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts @@ -87,6 +87,9 @@ export interface Dataset { } export interface ControlPanelState { + slice: { + slice_id: number; + }; form_data: QueryFormData; datasource: Dataset | QueryResponse | null; controls: ControlStateMapping; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/colorControls.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/colorControls.ts new file mode 100644 index 0000000000000..b9400d9d41529 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/colorControls.ts @@ -0,0 +1,32 @@ +/** + * 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. + */ +export const getColorControlsProps = (state: Record) => { + const dashboardId = state?.form_data?.dashboardId; + return { + chartId: state?.slice?.slice_id, + dashboardId, + hasDashboardColorScheme: + !!dashboardId && !!state?.form_data?.dashboard_color_scheme, + hasCustomLabelsColor: + Object.keys(state?.form_data?.label_colors || {}).length > 0, + colorNamespace: state?.form_data?.color_namespace, + mapLabelsColors: state?.form_data?.map_label_colors || {}, + sharedLabelsColors: state?.form_data?.shared_label_colors || [], + }; +}; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts index 77e883cafca90..48e551987180e 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts @@ -27,3 +27,4 @@ export * from './defineSavedMetrics'; export * from './getStandardizedControls'; export * from './getTemporalColumns'; export { default as displayTimeRelatedControls } from './displayTimeRelatedControls'; +export * from './colorControls'; diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/utils/colorControls.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/utils/colorControls.test.ts new file mode 100644 index 0000000000000..deadc3eedef30 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/utils/colorControls.test.ts @@ -0,0 +1,112 @@ +/** + * 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. + */ +import { getColorControlsProps } from '../../src'; + +describe('getColorControlsProps', () => { + it('should return default values when state is empty', () => { + const state = {}; + const result = getColorControlsProps(state); + expect(result).toEqual({ + chartId: undefined, + dashboardId: undefined, + hasDashboardColorScheme: false, + hasCustomLabelsColor: false, + colorNamespace: undefined, + mapLabelsColors: {}, + sharedLabelsColors: [], + }); + }); + + it('should return correct values when state has form_data with dashboardId and color scheme', () => { + const state = { + form_data: { + dashboardId: 123, + dashboard_color_scheme: 'blueScheme', + label_colors: {}, + }, + slice: { slice_id: 456 }, + }; + const result = getColorControlsProps(state); + expect(result).toEqual({ + chartId: 456, + dashboardId: 123, + hasDashboardColorScheme: true, + hasCustomLabelsColor: false, + colorNamespace: undefined, + mapLabelsColors: {}, + sharedLabelsColors: [], + }); + }); + + it('should detect custom label colors correctly', () => { + const state = { + form_data: { + dashboardId: 123, + label_colors: { label1: '#000000' }, + }, + slice: { slice_id: 456 }, + }; + const result = getColorControlsProps(state); + expect(result).toEqual({ + chartId: 456, + dashboardId: 123, + hasDashboardColorScheme: false, + hasCustomLabelsColor: true, + colorNamespace: undefined, + mapLabelsColors: {}, + sharedLabelsColors: [], + }); + }); + + it('should return shared label colors when available', () => { + const state = { + form_data: { + shared_label_colors: ['#FF5733', '#33FF57'], + }, + }; + const result = getColorControlsProps(state); + expect(result).toEqual({ + chartId: undefined, + dashboardId: undefined, + hasDashboardColorScheme: false, + hasCustomLabelsColor: false, + sharedLabelsColors: ['#FF5733', '#33FF57'], + colorNamespace: undefined, + mapLabelsColors: {}, + }); + }); + + it('should handle missing form_data and slice properties', () => { + const state = { + form_data: { + dashboardId: 789, + }, + }; + const result = getColorControlsProps(state); + expect(result).toEqual({ + chartId: undefined, + dashboardId: 789, + hasDashboardColorScheme: false, + hasCustomLabelsColor: false, + colorNamespace: undefined, + mapLabelsColors: {}, + sharedLabelsColors: [], + }); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts index 9c56d5114b9d9..9389ad549fd58 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts @@ -17,6 +17,7 @@ * under the License. */ +import { cloneDeep } from 'lodash'; import CategoricalColorScale from './CategoricalColorScale'; import { ColorsLookup } from './types'; import getCategoricalSchemeRegistry from './CategoricalSchemeRegistrySingleton'; @@ -37,10 +38,21 @@ export default class CategoricalColorNamespace { this.forcedItems = {}; } - getScale(schemeId?: string) { - const id = schemeId ?? getCategoricalSchemeRegistry().getDefaultKey() ?? ''; + /** + * A new CategoricalColorScale instance is created for each chart. + * + * @param colorScheme - the color scheme to use + * @returns a new instance of a color scale + */ + getScale(colorScheme?: string) { + const id = + colorScheme ?? getCategoricalSchemeRegistry().getDefaultKey() ?? ''; const scheme = getCategoricalSchemeRegistry().get(id); - return new CategoricalColorScale(scheme?.colors ?? [], this.forcedItems); + return new CategoricalColorScale( + scheme?.colors ?? [], + this.forcedItems, + colorScheme, + ); } /** @@ -59,6 +71,17 @@ export default class CategoricalColorNamespace { resetColors() { this.forcedItems = {}; } + + resetColorsForLabels(labels: string[] = []) { + const updatedForcedItems = cloneDeep(this.forcedItems); + labels.forEach(label => { + if (updatedForcedItems.hasOwnProperty(label)) { + delete updatedForcedItems[label]; + } + }); + + this.forcedItems = { ...updatedForcedItems }; + } } const namespaces: { @@ -80,16 +103,19 @@ export function getNamespace(name: string = DEFAULT_NAMESPACE) { export function getColor( value?: string, - schemeId?: string, + colorScheme?: string, namespace?: string, ) { - return getNamespace(namespace).getScale(schemeId).getColor(value); + return getNamespace(namespace).getScale(colorScheme).getColor(value); } /* Returns a new scale instance within the same namespace. Especially useful when a chart is booting for the first time + + @param scheme - the applied color scheme + @param namespace - the namespace */ -export function getScale(scheme?: string, namespace?: string) { - return getNamespace(namespace).getScale(scheme); +export function getScale(colorScheme?: string, namespace?: string) { + return getNamespace(namespace).getScale(colorScheme); } 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 f97f84cdec48f..707ae3d4afd66 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -21,14 +21,16 @@ import { scaleOrdinal, ScaleOrdinal } from 'd3-scale'; import { ExtensibleFunction } from '../models'; import { ColorsInitLookup, ColorsLookup } from './types'; import stringifyAndTrim from './stringifyAndTrim'; -import getLabelsColorMap from './LabelsColorMapSingleton'; +import getLabelsColorMap, { + LabelsColorMapSource, +} from './LabelsColorMapSingleton'; import { getAnalogousColors } from './utils'; import { FeatureFlag, isFeatureEnabled } from '../utils'; // Use type augmentation to correct the fact that // an instance of CategoricalScale is also a function interface CategoricalColorScale { - (x: { toString(): string }, y?: number, w?: string): string; + (x: { toString(): string }, y?: number): string; } class CategoricalColorScale extends ExtensibleFunction { @@ -50,11 +52,16 @@ class CategoricalColorScale extends ExtensibleFunction { * Constructor * @param {*} colors an array of colors * @param {*} forcedColors optional parameter that comes from parent - * (usually CategoricalColorNamespace) + * @param {*} appliedColorScheme the color scheme applied to the chart + * */ - constructor(colors: string[], forcedColors: ColorsInitLookup = {}) { - super((value: string, sliceId?: number, colorScheme?: string) => - this.getColor(value, sliceId, colorScheme), + constructor( + colors: string[], + forcedColors: ColorsInitLookup = {}, + appliedColorScheme?: string, + ) { + super((value: string, sliceId?: number) => + this.getColor(value, sliceId, appliedColorScheme), ); // holds original color scheme colors this.originColors = colors; @@ -107,15 +114,28 @@ class CategoricalColorScale extends ExtensibleFunction { * * @param value the value of a label to get the color for * @param sliceId the ID of the current chart - * @param colorScheme the original color scheme of the chart + * @param appliedColorScheme the color scheme applied to the chart * @returns the color or the next available color */ - getColor(value?: string, sliceId?: number, colorScheme?: string): string { + getColor( + value?: string, + sliceId?: number, + appliedColorScheme?: string, + ): string { const cleanedValue = stringifyAndTrim(value); - // priority: forced color (i.e. custom label colors) > shared color > scale color + // priority: forced color (aka custom label colors) > shared color > scale color const forcedColor = this.forcedColors?.[cleanedValue]; - const isExistingLabel = this.chartLabelsColorMap.has(cleanedValue); - let color = forcedColor || this.scale(cleanedValue); + const { source } = this.labelsColorMapInstance; + const currentColorMap = + source === LabelsColorMapSource.Dashboard + ? this.labelsColorMapInstance.getColorMap() + : this.chartLabelsColorMap; + const isExistingLabel = currentColorMap.has(cleanedValue); + let color = + forcedColor || + (isExistingLabel + ? (currentColorMap.get(cleanedValue) as string) + : this.scale(cleanedValue)); // a forced color will always be used independently of the usage count if (!forcedColor && !isExistingLabel) { @@ -128,7 +148,7 @@ class CategoricalColorScale extends ExtensibleFunction { this.isColorUsed(color) ) { // fallback to least used color - color = this.getNextAvailableColor(color); + color = this.getNextAvailableColor(cleanedValue, color); } } @@ -141,7 +161,7 @@ class CategoricalColorScale extends ExtensibleFunction { cleanedValue, color, sliceId, - colorScheme, + appliedColorScheme, ); } return color; @@ -164,48 +184,76 @@ class CategoricalColorScale extends ExtensibleFunction { * @param color the color to check * @returns the count of the color usage in this slice */ - getColorUsageCount(currentColor: string): number { - let count = 0; - this.chartLabelsColorMap.forEach(color => { - if (color === currentColor) { - count += 1; - } - }); - return count; + getColorUsageCount(color: string): number { + return Array.from(this.chartLabelsColorMap.values()).filter( + value => value === color, + ).length; } /** - * Lower chances of color collision by returning the least used color - * Checks across colors of current slice within LabelsColorMapSingleton + * Lower chances of color collision by returning the least used color. + * Checks across colors of current slice within chartLabelsColorMap. * + * @param currentLabel the current label * @param currentColor the current color - * @returns the least used color that is not the excluded color + * @returns the least used color that is not the current color */ - getNextAvailableColor(currentColor: string) { - const colorUsageArray = this.colors.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, + getNextAvailableColor(currentLabel: string, currentColor: string): string { + // Precompute color usage counts for all colors + const colorUsageCounts = new Map( + this.colors.map(color => [color, this.getColorUsageCount(color)]), ); - // fallback to currentColor color - if (!otherColors.length || hasNoneAvailable) { - return currentColor; + // Get an ordered array of labels from the map + const orderedLabels = Array.from(this.chartLabelsColorMap.keys()); + const currentLabelIndex = orderedLabels.indexOf(currentLabel); + + // Helper to infer "previous" and "next" labels based on index + const getAdjacentLabelsColors = (): string[] => { + const previousLabel = + currentLabelIndex > 0 ? orderedLabels[currentLabelIndex - 1] : null; + const nextLabel = + currentLabelIndex < orderedLabels.length - 1 + ? orderedLabels[currentLabelIndex + 1] + : null; + + const previousColor = previousLabel + ? this.chartLabelsColorMap.get(previousLabel) + : null; + const nextColor = nextLabel + ? this.chartLabelsColorMap.get(nextLabel) + : null; + + return [previousColor, nextColor].filter(color => color) as string[]; + }; + + const adjacentColors = getAdjacentLabelsColors(); + + // Determine adjusted score (usage count + penalties) + const calculateScore = (color: string): number => { + /* istanbul ignore next */ + const usageCount = colorUsageCounts.get(color) || 0; + const adjacencyPenalty = adjacentColors.includes(color) ? 100 : 0; + return usageCount + adjacencyPenalty; + }; + + // If there is any color that has never been used, prioritize it + const unusedColor = this.colors.find( + color => (colorUsageCounts.get(color) || 0) === 0, + ); + if (unusedColor) { + return unusedColor; } - // Finding the least used color - const leastUsedColor = otherColors.reduce((min, entry) => - entry.count < min.count ? entry : min, - ).color; + // If all colors are used, calculate scores and choose the best one + const otherColors = this.colors.filter(color => color !== currentColor); - return leastUsedColor; + // Find the color with the minimum score, defaulting to currentColor + return otherColors.reduce((bestColor, color) => { + const bestScore = calculateScore(bestColor); + const currentScore = calculateScore(color); + return currentScore < bestScore ? color : bestColor; + }, currentColor); } /** diff --git a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts index 59d3f8cc5de79..cf50d6f6a252e 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts @@ -18,6 +18,7 @@ */ import { makeSingleton } from '../utils'; +import CategoricalColorNamespace from './CategoricalColorNamespace'; export enum LabelsColorMapSource { Dashboard, @@ -25,7 +26,10 @@ export enum LabelsColorMapSource { } export class LabelsColorMap { - chartsLabelsMap: Map; + chartsLabelsMap: Map< + number, + { labels: string[]; scheme?: string; ownScheme?: string } + >; colorMap: Map; @@ -38,17 +42,38 @@ export class LabelsColorMap { this.source = LabelsColorMapSource.Dashboard; } - updateColorMap(categoricalNamespace: any, colorScheme?: string) { - const newColorMap = new Map(); - this.colorMap.clear(); + /** + * Wipes out the color map and updates it with the new color scheme. + * + * @param categoricalNamespace - the namespace to use for color mapping + * @param colorScheme - color scheme + */ + updateColorMap( + categoricalNamespace: CategoricalColorNamespace, + colorScheme?: string, + merge = false, + ) { + const newColorMap = this.colorMap; + + if (!merge) { + newColorMap.clear(); + } + this.chartsLabelsMap.forEach((chartConfig, sliceId) => { - const { labels, scheme: originalChartColorScheme } = chartConfig; - const currentColorScheme = colorScheme || originalChartColorScheme; - const colorScale = categoricalNamespace.getScale(currentColorScheme); + const { labels, ownScheme } = chartConfig; + const appliedColorScheme = colorScheme || ownScheme; + const colorScale = categoricalNamespace.getScale(appliedColorScheme); labels.forEach(label => { - const newColor = colorScale.getColor(label, sliceId); - newColorMap.set(label, newColor); + // if merge, apply the scheme only to new labels in the map + if (!merge || !this.colorMap.has(label)) { + const newColor = colorScale.getColor( + label, + sliceId, + appliedColorScheme, + ); + newColorMap.set(label, newColor); + } }); }); this.colorMap = newColorMap; @@ -58,29 +83,63 @@ export class LabelsColorMap { return this.colorMap; } + /** + * + * Called individually by each plugin via getColor fn. + * + * @param label - the label name + * @param color - the color + * @param sliceId - the chart id + * @param colorScheme - the color scheme + * + */ addSlice( label: string, color: string, sliceId: number, colorScheme?: string, ) { - if (this.source !== LabelsColorMapSource.Dashboard) return; - const chartConfig = this.chartsLabelsMap.get(sliceId) || { labels: [], - scheme: '', + scheme: undefined, + ownScheme: undefined, }; + const { labels } = chartConfig; if (!labels.includes(label)) { labels.push(label); this.chartsLabelsMap.set(sliceId, { labels, scheme: colorScheme, + ownScheme: chartConfig.ownScheme, + }); + } + if (this.source === LabelsColorMapSource.Dashboard) { + this.colorMap.set(label, color); + } + } + + /** + * Used to make sure all slices respect their original scheme. + * + * @param sliceId - the chart id + * @param ownScheme - the color scheme + */ + setOwnColorScheme(sliceId: number, ownScheme: string) { + const chartConfig = this.chartsLabelsMap.get(sliceId); + if (chartConfig) { + this.chartsLabelsMap.set(sliceId, { + ...chartConfig, + ownScheme, }); } - this.colorMap.set(label, color); } + /** + * Remove a slice from the color map. + * + * @param sliceId - the chart + */ removeSlice(sliceId: number) { if (this.source !== LabelsColorMapSource.Dashboard) return; @@ -96,10 +155,20 @@ export class LabelsColorMap { this.colorMap = newColorMap; } + /** + * Clear the shared labels color map. + */ clear() { - this.chartsLabelsMap.clear(); this.colorMap.clear(); } + + /** + * Clears all maps + */ + reset() { + this.clear(); + this.chartsLabelsMap.clear(); + } } const getInstance = makeSingleton(LabelsColorMap); 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 69fb38eea3f3f..014be2548644d 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 @@ -161,4 +161,63 @@ describe('CategoricalColorNamespace', () => { expect(color).toBe(color2); }); }); + describe('statis resetColorsForLabels(labels)', () => { + it('removes specified labels from forcedItems', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.setColor('label2', 'blue'); + namespace.resetColorsForLabels(['label1']); + + expect(namespace.forcedItems).toMatchObject({ label2: 'blue' }); + }); + it('does not modify forcedItems if no labels are provided', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.setColor('label2', 'blue'); + namespace.resetColorsForLabels(); + + expect(namespace.forcedItems).toMatchObject({ + label1: 'red', + label2: 'blue', + }); + }); + it('does nothing if the label is not in forcedItems', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.resetColorsForLabels(['label2']); // label2 doesn't exist + + expect(namespace.forcedItems).toMatchObject({ label1: 'red' }); + }); + it('removes all labels when all are provided', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.setColor('label2', 'blue'); + namespace.resetColorsForLabels(['label1', 'label2']); + + expect(namespace.forcedItems).toMatchObject({}); + }); + it('creates a deep copy of forcedItems before modifying', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + + const originalForcedItems = namespace.forcedItems; + namespace.resetColorsForLabels(['label1']); + + expect(originalForcedItems).not.toBe(namespace.forcedItems); + }); + it('removes the label if it exists in updatedForcedItems', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.resetColorsForLabels(['label1']); + + expect(namespace.forcedItems).toEqual({}); + }); + it('does nothing for a label not in updatedForcedItems', () => { + const namespace = getNamespace('test-reset-individual'); + namespace.setColor('label1', 'red'); + namespace.resetColorsForLabels(['label2']); // label2 doesn't exist + + expect(namespace.forcedItems).toEqual({ label1: 'red' }); + }); + }); }); 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 97d756cb04321..9ba4bcc5b01a4 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,7 +18,11 @@ */ import { ScaleOrdinal } from 'd3-scale'; -import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core'; +import { + CategoricalColorScale, + FeatureFlag, + LabelsColorMapSource, +} from '@superset-ui/core'; describe('CategoricalColorScale', () => { beforeEach(() => { @@ -43,7 +47,6 @@ describe('CategoricalColorScale', () => { expect(scale).toBeInstanceOf(CategoricalColorScale); expect(scale.forcedColors).toBe(forcedColors); }); - it('can refer to colors based on their index', () => { const forcedColors = { pig: 1, horse: 5 }; const scale = new CategoricalColorScale( @@ -67,7 +70,7 @@ describe('CategoricalColorScale', () => { >; let getNextAvailableColorSpy: jest.SpyInstance< string, - [currentColor: string] + [currentLabel: string, currentColor: string] >; beforeEach(() => { @@ -83,6 +86,36 @@ describe('CategoricalColorScale', () => { jest.restoreAllMocks(); }); + it('uses labelsColorMapInstance color map when source is Dashboard, otherwise uses chartLabelsColorMap', () => { + const sliceId = 123; + const colorScheme = 'preset'; + + // Mock chartLabelsColorMap and labelsColorMapInstance's getColorMap + const chartColorMap = new Map([['testValueChart', 'chartColor']]); + const dashboardColorMap = new Map([['testValueDash', 'dashboardColor']]); + scale.chartLabelsColorMap = chartColorMap; + jest + .spyOn(scale.labelsColorMapInstance, 'getColorMap') + .mockReturnValue(dashboardColorMap); + + // Test when source is Dashboard + scale.labelsColorMapInstance.source = LabelsColorMapSource.Dashboard; + const colorFromDashboard = scale.getColor( + 'testValueDash', + sliceId, + colorScheme, + ); + expect(colorFromDashboard).toBe('dashboardColor'); + + // Test when source is not Dashboard + scale.labelsColorMapInstance.source = LabelsColorMapSource.Explore; + const colorFromChart = scale.getColor( + 'testValueChart', + sliceId, + colorScheme, + ); + expect(colorFromChart).toBe('chartColor'); + }); it('returns same color for same value', () => { const scale = new CategoricalColorScale(['blue', 'red', 'green'], { pig: 'red', @@ -177,7 +210,10 @@ describe('CategoricalColorScale', () => { scale.getColor('testValue3'); scale.getColor('testValue4'); - expect(getNextAvailableColorSpy).toHaveBeenCalledWith('blue'); + expect(getNextAvailableColorSpy).toHaveBeenCalledWith( + 'testValue4', + 'blue', + ); getNextAvailableColorSpy.mockClear(); @@ -289,23 +325,25 @@ describe('CategoricalColorScale', () => { }); }); - describe('.getNextAvailableColor(currentColor)', () => { + describe('.getNextAvailableColor(currentLabel, 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'); // Since 'green' hasn't been used, it's considered the least used. - expect(scale.getNextAvailableColor('blue')).toBe('green'); + expect(scale.getNextAvailableColor('fish', 'blue')).toBe('green'); }); - it('handles cases where all colors are equally used and returns the current color', () => { + it('returns the least used color among all', () => { 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'); + scale.getColor('puppy'); // blue + scale.getColor('teddy'); // red + // All colors used, so the function should return least used + expect(scale.getNextAvailableColor('darling', 'red')).toBe('green'); }); it('returns the least used color accurately even when some colors are used more frequently', () => { @@ -324,7 +362,57 @@ describe('CategoricalColorScale', () => { scale.getColor('pony'); // green // Yellow is the least used color, so it should be returned. - expect(scale.getNextAvailableColor('blue')).toBe('yellow'); + expect(scale.getNextAvailableColor('pony', 'blue')).toBe('yellow'); + }); + it('does not return adjacent colors if a non-adjacent color is equally used', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + scale.chartLabelsColorMap.set('label1', 'red'); // Adjacent + scale.chartLabelsColorMap.set('label2', 'blue'); // currentLabel + scale.chartLabelsColorMap.set('label3', 'green'); // Adjacent + + // Green and blue are equally used, but green is adjacent and penalized. + expect(scale.getNextAvailableColor('label2', 'blue')).toBe('blue'); + }); + it('prioritizes a color that has never been used, even if there are adjacent colors', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + scale.getColor('cat'); // blue + scale.getColor('dog'); // red + + scale.chartLabelsColorMap.set('label1', 'red'); + scale.chartLabelsColorMap.set('label2', 'blue'); // currentLabel + + // Green has never been used, so it is prioritized. + expect(scale.getNextAvailableColor('label2', 'blue')).toBe('green'); + }); + it('returns the least used or unused color when there are no adjacent labels', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + scale.getColor('cat'); // blue + scale.getColor('dog'); // red + + // No adjacent labels are defined in chartLabelsColorMap. + expect(scale.getNextAvailableColor('label2', 'green')).toBe('green'); + }); + it('handles colors that have never been used (fallback to usage count 0)', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + + // Do not use "green" at all + scale.getColor('cat'); // blue + scale.getColor('dog'); // red + + // "green" has never been used, so usageCount for "green" should fallback to 0 + expect(scale.getNextAvailableColor('label2', 'red')).toBe('green'); + }); + it('handles a color with an explicit usage count of 0', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + + // Mock or override getColorUsageCount to return 0 for "blue" + jest.spyOn(scale, 'getColorUsageCount').mockImplementation(color => { + if (color === 'blue') return 0; // Explicitly return 0 for "blue" + return 1; // Return 1 for other colors + }); + + // "blue" should still be a valid option with a usage count of 0 + expect(scale.getNextAvailableColor('label1', 'red')).toBe('blue'); }); }); diff --git a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts index 17efe6692f3b5..24521d2d9dbb0 100644 --- a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts @@ -53,7 +53,7 @@ describe('LabelsColorMap', () => { beforeEach(() => { getLabelsColorMap().source = LabelsColorMapSource.Dashboard; - getLabelsColorMap().clear(); + getLabelsColorMap().reset(); }); it('has default value out-of-the-box', () => { @@ -92,11 +92,17 @@ describe('LabelsColorMap', () => { expect(Object.fromEntries(colorMap)).toEqual({ b: 'green' }); }); - it('should do nothing when source is not dashboard', () => { + it('should set a new color only when source is dashboard', () => { const labelsColorMap = getLabelsColorMap(); labelsColorMap.source = LabelsColorMapSource.Explore; labelsColorMap.addSlice('a', 'red', 1); - expect(Object.fromEntries(labelsColorMap.chartsLabelsMap)).toEqual({}); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({}); + + labelsColorMap.source = LabelsColorMapSource.Dashboard; + labelsColorMap.addSlice('a', 'red', 1); + const colorMap2 = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap2)).toEqual({ a: 'red' }); }); }); @@ -126,7 +132,7 @@ describe('LabelsColorMap', () => { }); }); - describe('.updateColorMap(namespace, scheme)', () => { + describe('.updateColorMap(namespace, scheme, merge)', () => { let categoricalNamespace: any; let mockedNamespace: any; let labelsColorMap: any; @@ -141,18 +147,24 @@ describe('LabelsColorMap', () => { }; }); + it('should clear color map when not merge', () => { + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.updateColorMap(mockedNamespace, 'testColors2', false); + expect(labelsColorMap.colorMap).toEqual(new Map([['a', 'mockColor']])); + }); + + it('should not clear color map when merge', () => { + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.updateColorMap(mockedNamespace, 'testColors2', true); + expect(labelsColorMap.colorMap).not.toEqual(new Map()); + }); + it('should use provided color scheme', () => { labelsColorMap.addSlice('a', 'red', 1); labelsColorMap.updateColorMap(mockedNamespace, 'testColors2'); expect(mockedNamespace.getScale).toHaveBeenCalledWith('testColors2'); }); - it('should fallback to original chart color scheme if no color scheme is provided', () => { - labelsColorMap.addSlice('a', 'red', 1, 'originalScheme'); - labelsColorMap.updateColorMap(mockedNamespace); - expect(mockedNamespace.getScale).toHaveBeenCalledWith('originalScheme'); - }); - it('should fallback to undefined if no color scheme is provided', () => { labelsColorMap.addSlice('a', 'red', 1); labelsColorMap.addSlice('b', 'blue', 2); @@ -181,6 +193,23 @@ describe('LabelsColorMap', () => { }); }); + it('should update only new labels in the color map when merge', () => { + labelsColorMap.colorMap = new Map(); + + labelsColorMap.addSlice('a', 'yellow', 1); + labelsColorMap.addSlice('b', 'green', 1); + labelsColorMap.addSlice('c', 'purple', 1); + + labelsColorMap.updateColorMap(categoricalNamespace, 'testColors2', true); + + const mergedColorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(mergedColorMap)).toEqual({ + a: 'yellow', + b: 'green', + c: 'purple', + }); + }); + it('should use recycle colors', () => { window.featureFlags = { [FeatureFlag.UseAnalagousColors]: false, @@ -231,4 +260,47 @@ describe('LabelsColorMap', () => { expect(Object.fromEntries(colorMap)).toEqual({}); }); }); + + describe('setOwnColorScheme(sliceId, ownScheme)', () => { + it('should update the scheme in the config', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.source = LabelsColorMapSource.Explore; + const sliceId = 1; + const initialConfig = { labels: ['initial config'] }; + + labelsColorMap.chartsLabelsMap = new Map(); + labelsColorMap.chartsLabelsMap.set(sliceId, initialConfig); + + labelsColorMap.setOwnColorScheme(sliceId, 'newScheme'); + + expect(labelsColorMap.chartsLabelsMap.get(sliceId)).toEqual({ + ...initialConfig, + ownScheme: 'newScheme', + }); + }); + it('should update ownScheme when source is not Explore', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.source = LabelsColorMapSource.Dashboard; + const sliceId = 1; + const initialConfig = { labels: ['initial config'] }; + + labelsColorMap.chartsLabelsMap = new Map(); + labelsColorMap.chartsLabelsMap.set(sliceId, initialConfig); + + labelsColorMap.setOwnColorScheme(sliceId, 'newScheme'); + + expect(labelsColorMap.chartsLabelsMap.get(sliceId)).toEqual({ + ...initialConfig, + ownScheme: 'newScheme', + }); + }); + it('should do nothing when chart config does not exist', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.source = LabelsColorMapSource.Explore; + const sliceId = 1; + + labelsColorMap.setOwnColorScheme(sliceId, 'newScheme'); + expect(labelsColorMap.chartsLabelsMap.get(sliceId)).toEqual(undefined); + }); + }); }); diff --git a/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js index f947e9ff697d8..1d5ed45683fb8 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js +++ b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js @@ -93,7 +93,7 @@ function Chord(element, props) { .append('path') .attr('id', (d, i) => `group${i}`) .attr('d', arc) - .style('fill', (d, i) => colorFn(nodes[i], sliceId, colorScheme)); + .style('fill', (d, i) => colorFn(nodes[i], sliceId)); // Add a text label. const groupText = group.append('text').attr('x', 6).attr('dy', 15); @@ -121,7 +121,7 @@ function Chord(element, props) { .on('mouseover', d => { chord.classed('fade', p => p !== d); }) - .style('fill', d => colorFn(nodes[d.source.index], sliceId, colorScheme)) + .style('fill', d => colorFn(nodes[d.source.index], sliceId)) .attr('d', path); // Add an elaborate mouseover title for each chord. diff --git a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js index 61ca6cc2fe76b..b1cf5016dcf70 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js +++ b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js @@ -37,6 +37,7 @@ const propTypes = { width: PropTypes.number, height: PropTypes.number, country: PropTypes.string, + colorScheme: PropTypes.string, linearColorScheme: PropTypes.string, mapBaseUrl: PropTypes.string, numberFormat: PropTypes.string, diff --git a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx index 0af5f8bf776b7..c14b83c1ca84c 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx +++ b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx @@ -73,12 +73,11 @@ class CustomHistogram extends PureComponent { showLegend, sliceId, } = this.props; - const colorFn = CategoricalColorNamespace.getScale(colorScheme); const keys = data.map(d => d.key); const colorScale = scaleOrdinal({ domain: keys, - range: keys.map(x => colorFn(x, sliceId, colorScheme)), + range: keys.map(x => colorFn(x, sliceId)), }); return ( diff --git a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js index a71bb63d698eb..fddc0f928d8a3 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js +++ b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js @@ -384,7 +384,7 @@ function Icicle(element, props) { // Apply color scheme g.selectAll('rect').style('fill', d => { - d.color = colorFn(d.name, sliceId, colorScheme); + d.color = colorFn(d.name, sliceId); return d.color; }); diff --git a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js index e54fc0b6c542f..93d402cb61bed 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js +++ b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js @@ -46,6 +46,7 @@ const propTypes = { numberFormat: PropTypes.string, useRichTooltip: PropTypes.bool, useAreaProportions: PropTypes.bool, + colorScheme: PropTypes.string, }; function copyArc(d) { @@ -120,14 +121,14 @@ function Rose(element, props) { .map(v => ({ key: v.name, value: v.value, - color: colorFn(v.name, sliceId, colorScheme), + color: colorFn(v.name, sliceId), highlight: v.id === d.arcId, })) : [ { key: d.name, value: d.val, - color: colorFn(d.name, sliceId, colorScheme), + color: colorFn(d.name, sliceId), }, ]; @@ -138,7 +139,7 @@ function Rose(element, props) { }; } - legend.width(width).color(d => colorFn(d.key, sliceId, colorScheme)); + legend.width(width).color(d => colorFn(d.key, sliceId)); legendWrap.datum(legendData(datum)).call(legend); tooltip.headerFormatter(timeFormat).valueFormatter(format); @@ -385,7 +386,7 @@ function Rose(element, props) { const arcs = ae .append('path') .attr('class', 'arc') - .attr('fill', d => colorFn(d.name, sliceId, colorScheme)) + .attr('fill', d => colorFn(d.name, sliceId)) .attr('d', arc); function mousemove() { diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js index 00f47ada2666e..c9fe27eb23510 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js @@ -83,7 +83,7 @@ function computeGraph(links) { function SankeyLoop(element, props) { const { data, width, height, colorScheme, sliceId } = props; - const color = CategoricalColorNamespace.getScale(colorScheme); + const colorFn = CategoricalColorNamespace.getScale(colorScheme); const margin = { ...defaultMargin, ...props.margin }; const innerWidth = width - margin.left - margin.right; const innerHeight = height - margin.top - margin.bottom; @@ -107,7 +107,7 @@ function SankeyLoop(element, props) { value / sValue, )})`, ) - .linkColor(d => color(d.source.name, sliceId)); + .linkColor(d => colorFn(d.source.name, sliceId)); const div = select(element); div.selectAll('*').remove(); diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js index 0639edad45c0b..a38142c564680 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js @@ -67,7 +67,6 @@ function Sankey(element, props) { .attr('height', innerHeight + margin.top + margin.bottom) .append('g') .attr('transform', `translate(${margin.left},${margin.top})`); - const colorFn = CategoricalColorNamespace.getScale(colorScheme); const sankey = d3Sankey() @@ -219,7 +218,7 @@ function Sankey(element, props) { .attr('width', sankey.nodeWidth()) .style('fill', d => { const name = d.name || 'N/A'; - d.color = colorFn(name, sliceId, colorScheme); + d.color = colorFn(name, sliceId); return d.color; }) diff --git a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js index 6b69c6b2d8c3e..03ea4ea9c78ba 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js +++ b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js @@ -43,6 +43,7 @@ const propTypes = { showBubbles: PropTypes.bool, linearColorScheme: PropTypes.string, color: PropTypes.string, + colorScheme: PropTypes.string, setDataMask: PropTypes.func, onContextMenu: PropTypes.func, emitCrossFilters: PropTypes.bool, @@ -85,24 +86,24 @@ function WorldMap(element, props) { .range([1, maxBubbleSize]); let processedData; - let colorScale; + let colorFn; if (colorBy === ColorBy.Country) { - colorScale = CategoricalColorNamespace.getScale(colorScheme); + colorFn = CategoricalColorNamespace.getScale(colorScheme); processedData = filteredData.map(d => ({ ...d, radius: radiusScale(Math.sqrt(d.m2)), - fillColor: colorScale(d.name, sliceId), + fillColor: colorFn(d.name, sliceId), })); } else { - colorScale = getSequentialSchemeRegistry() + colorFn = getSequentialSchemeRegistry() .get(linearColorScheme) .createLinearScale(d3Extent(filteredData, d => d.m1)); processedData = filteredData.map(d => ({ ...d, radius: radiusScale(Math.sqrt(d.m2)), - fillColor: colorScale(d.m1), + fillColor: colorFn(d.m1), })); } diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx index 7dff2af2214a0..208ca6e0f5f3b 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx @@ -52,16 +52,14 @@ const { getScale } = CategoricalColorNamespace; function getCategories(fd: QueryFormData, data: JsonObject[]) { const c = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 }; const fixedColor = [c.r, c.g, c.b, 255 * c.a]; - const colorFn = getScale(fd.color_scheme); + const appliedScheme = fd.color_scheme; + const colorFn = getScale(appliedScheme); const categories = {}; data.forEach(d => { if (d.cat_color != null && !categories.hasOwnProperty(d.cat_color)) { let color; if (fd.dimension) { - color = hexToRGB( - colorFn(d.cat_color, fd.sliceId, fd.color_scheme), - c.a * 255, - ); + color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); } else { color = fixedColor; } @@ -132,15 +130,13 @@ const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => { const addColor = useCallback((data: JsonObject[], fd: QueryFormData) => { const c = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 }; - const colorFn = getScale(fd.color_scheme); + const appliedScheme = fd.color_scheme; + const colorFn = getScale(appliedScheme); return data.map(d => { let color; if (fd.dimension) { - color = hexToRGB( - colorFn(d.cat_color, fd.sliceId, fd.color_scheme), - c.a * 255, - ); + color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); return { ...d, color }; } diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx index ee5ae6c85fe06..242a8e0650247 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx @@ -55,7 +55,8 @@ export function getLayer( setTooltip: (tooltip: TooltipProps['tooltip']) => void, ) { const fd = formData; - const colorScale = CategoricalColorNamespace.getScale(fd.color_scheme); + const appliedScheme = fd.color_scheme; + const colorScale = CategoricalColorNamespace.getScale(appliedScheme); const colorRange = colorScale .range() .map(color => hexToRGB(color)) as Color[]; diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Hex/Hex.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Hex/Hex.tsx index 84100da7586d8..93df7f0b1d265 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Hex/Hex.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Hex/Hex.tsx @@ -54,7 +54,8 @@ export function getLayer( setTooltip: (tooltip: TooltipProps['tooltip']) => void, ) { const fd = formData; - const colorScale = CategoricalColorNamespace.getScale(fd.color_scheme); + const appliedScheme = fd.color_scheme; + const colorScale = CategoricalColorNamespace.getScale(appliedScheme); const colorRange = colorScale .range() .map(color => hexToRGB(color)) as Color[]; diff --git a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js index 06455f16d8b2b..a454fe28b4644 100644 --- a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js +++ b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js @@ -658,9 +658,7 @@ function nvd3Vis(element, props) { } else if (vizType !== 'bullet') { const colorFn = getScale(colorScheme); chart.color( - d => - d.color || - colorFn(cleanColorInput(d[colorKey]), sliceId, colorScheme), + d => d.color || colorFn(cleanColorInput(d[colorKey]), sliceId), ); } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts index 5eb80b1a6fed5..511d083d47915 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts @@ -109,9 +109,9 @@ export default function transformProps( datum[`${metric}__outliers`], ], itemStyle: { - color: colorFn(groupbyLabel, sliceId, colorScheme), + color: colorFn(groupbyLabel, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : 0.6, - borderColor: colorFn(groupbyLabel, sliceId, colorScheme), + borderColor: colorFn(groupbyLabel, sliceId), }, }; }); @@ -150,7 +150,7 @@ export default function transformProps( }, }, itemStyle: { - color: colorFn(groupbyLabel, sliceId, colorScheme), + color: colorFn(groupbyLabel, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts index cc9fb5f79ce94..f1a62ab7de055 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts @@ -108,8 +108,8 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) { legendOrientation, legendMargin, legendType, + sliceId, }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData }; - const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); const legends = new Set(); @@ -138,7 +138,10 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) { ], ], type: 'scatter', - itemStyle: { color: colorFn(name), opacity }, + itemStyle: { + color: colorFn(name, sliceId), + opacity, + }, }); legends.add(name); }); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts index d0be236fda321..cd2a3a0ffbfb8 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts @@ -145,7 +145,6 @@ export default function transformProps( }, {}); const { setDataMask = () => {}, onContextMenu } = hooks; - const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); const numberFormatter = getValueFormatter( metric, @@ -175,7 +174,7 @@ export default function transformProps( value, name, itemStyle: { - color: colorFn(name, sliceId, colorScheme), + color: colorFn(name, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts index 0bea236b41831..e2dd9d4880900 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts @@ -166,6 +166,7 @@ export default function transformProps( const name = groupbyLabels .map(column => `${verboseMap[column] || column}: ${data_point[column]}`) .join(', '); + const colorLabel = groupbyLabels.map(col => data_point[col] as string); columnsLabelMap.set( name, groupbyLabels.map(col => data_point[col] as string), @@ -174,7 +175,7 @@ export default function transformProps( value: data_point[metricLabel] as number, name, itemStyle: { - color: colorFn(index, sliceId, colorScheme), + color: colorFn(colorLabel, sliceId), }, title: { offsetCenter: [ @@ -202,7 +203,7 @@ export default function transformProps( item = { ...item, itemStyle: { - color: colorFn(index, sliceId, colorScheme), + color: colorFn(index, sliceId), opacity: OpacityEnum.SemiTransparent, }, detail: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts index 100d21349e416..c98d6160921b7 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts @@ -297,14 +297,15 @@ export default function transformProps( }); const categoryList = [...categories]; - const series: GraphSeriesOption[] = [ { zoom: DEFAULT_GRAPH_SERIES_OPTION.zoom, type: 'graph', categories: categoryList.map(c => ({ name: c, - itemStyle: { color: colorFn(c, sliceId, colorScheme) }, + itemStyle: { + color: colorFn(c, sliceId), + }, })), layout, force: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts index 0b6667c0c7861..b02f86b232746 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts @@ -191,7 +191,6 @@ export default function transformProps( }, {}); const { setDataMask = () => {}, onContextMenu } = hooks; - const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); const numberFormatter = getValueFormatter( metric, @@ -223,7 +222,7 @@ export default function transformProps( value, name, itemStyle: { - color: colorFn(name, sliceId, colorScheme), + color: colorFn(name, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts index acadebd5ec110..7b7af34ae5f69 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts @@ -109,7 +109,6 @@ export default function transformProps( ...formData, }; const { setDataMask = () => {}, onContextMenu } = hooks; - const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); const numberFormatter = getNumberFormatter(numberFormat); const formatter = (params: CallbackDataParams) => @@ -182,7 +181,7 @@ export default function transformProps( value: metricLabels.map(metricLabel => datum[metricLabel]), name: joinedName, itemStyle: { - color: colorFn(joinedName, sliceId, colorScheme), + color: colorFn(joinedName, sliceId), opacity: isFiltered ? OpacityEnum.Transparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Sankey/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Sankey/transformProps.ts index 00ba97821f45e..c3db5052bf122 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Sankey/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Sankey/transformProps.ts @@ -41,7 +41,7 @@ export default function transformProps( const refs: Refs = {}; const { formData, height, hooks, queriesData, width } = chartProps; const { onLegendStateChanged } = hooks; - const { colorScheme, metric, source, target } = formData; + const { colorScheme, metric, source, target, sliceId } = formData; const { data } = queriesData[0]; const colorFn = CategoricalColorNamespace.getScale(colorScheme); const metricLabel = getMetricLabel(metric); @@ -68,7 +68,7 @@ export default function transformProps( ).map(name => ({ name, itemStyle: { - color: colorFn(name), + color: colorFn(name, sliceId), }, })); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index b552ceba5c49e..435dc0123f5f1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -202,7 +202,6 @@ export default function transformProps( } return { ...acc, [entry[0]]: entry[1] }; }, {}); - const colorScale = CategoricalColorNamespace.getScale(colorScheme as string); const rebasedData = rebaseForecastDatum(data, verboseMap); let xAxisLabel = getXAxisLabel(chartProps.rawFormData) as string; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts index bdd23c3fc3880..70755574fba75 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts @@ -176,18 +176,18 @@ export default function transformProps( let item: TreemapSeriesNodeItemOption = { name, value, + colorSaturation: COLOR_SATURATION, + itemStyle: { + borderColor: BORDER_COLOR, + color: colorFn(name, sliceId), + borderWidth: BORDER_WIDTH, + gapWidth: GAP_WIDTH, + }, }; if (treeNode.children?.length) { item = { ...item, children: traverse(treeNode.children, newPath), - colorSaturation: COLOR_SATURATION, - itemStyle: { - borderColor: BORDER_COLOR, - color: colorFn(name, sliceId, colorScheme), - borderWidth: BORDER_WIDTH, - gapWidth: GAP_WIDTH, - }, }; } else { const joinedName = newPath.join(','); @@ -217,7 +217,7 @@ export default function transformProps( colorSaturation: COLOR_SATURATION, itemStyle: { borderColor: BORDER_COLOR, - color: colorFn(`${metricLabel}`, sliceId, colorScheme), + color: colorFn(`${metricLabel}`, sliceId), borderWidth: BORDER_WIDTH, gapWidth: GAP_WIDTH, }, diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx index 53bf98fe90af7..6bdda4a3d11af 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx @@ -29,7 +29,7 @@ import { SupersetThemeProps, withTheme, seed, - CategoricalColorScale, + CategoricalColorNamespace, } from '@superset-ui/core'; import { isEqual } from 'lodash'; @@ -230,7 +230,7 @@ class WordCloud extends PureComponent { encoder.channels.color.setDomainFromDataset(words); const { getValueFromDatum } = encoder.channels.color; - const colorFn = encoder.channels.color.scale as CategoricalColorScale; + const colorFn = CategoricalColorNamespace.getScale(colorScheme); const viewBoxWidth = width * scaleFactor; const viewBoxHeight = height * scaleFactor; @@ -250,11 +250,7 @@ class WordCloud extends PureComponent { fontSize={`${w.size}px`} fontWeight={w.weight} fontFamily={w.font} - fill={colorFn( - getValueFromDatum(w) as string, - sliceId, - colorScheme, - )} + fill={colorFn(getValueFromDatum(w) as string, sliceId)} textAnchor="middle" transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`} > diff --git a/superset-frontend/spec/fixtures/mockDashboardFormData.ts b/superset-frontend/spec/fixtures/mockDashboardFormData.ts index a1adb18a7e722..3f089802509f5 100644 --- a/superset-frontend/spec/fixtures/mockDashboardFormData.ts +++ b/superset-frontend/spec/fixtures/mockDashboardFormData.ts @@ -26,10 +26,7 @@ export const getDashboardFormData = (overrides: JsonObject = {}) => ({ girl: '#FF69B4', boy: '#ADD8E6', }, - shared_label_colors: { - boy: '#ADD8E6', - girl: '#FF69B4', - }, + shared_label_colors: ['boy', 'girl'], color_scheme: 'd3Category20b', extra_filters: [ { diff --git a/superset-frontend/spec/fixtures/mockDashboardState.js b/superset-frontend/spec/fixtures/mockDashboardState.js index 737e38aef59e0..42360cdc7142e 100644 --- a/superset-frontend/spec/fixtures/mockDashboardState.js +++ b/superset-frontend/spec/fixtures/mockDashboardState.js @@ -113,6 +113,6 @@ export const overwriteConfirmMetadata = { slug: null, owners: [], json_metadata: - '{"timed_refresh_immune_slices":[],"expanded_slices":{},"refresh_frequency":0,"default_filters":"{}","color_scheme":"supersetColors","label_colors":{"0":"#FCC700","1":"#A868B7","15":"#3CCCCB","30":"#A38F79","45":"#8FD3E4","age":"#1FA8C9","Yes,":"#1FA8C9","Female":"#454E7C","Prefer":"#5AC189","No,":"#FF7F44","Male":"#666666","Prefer not to say":"#E04355","Ph.D.":"#FCC700","associate\'s degree":"#A868B7","bachelor\'s degree":"#3CCCCB","high school diploma or equivalent (GED)":"#A38F79","master\'s degree (non-professional)":"#8FD3E4","no high school (secondary school)":"#A1A6BD","professional degree (MBA, MD, JD, etc.)":"#ACE1C4","some college credit, no degree":"#FEC0A1","some high school":"#B2B2B2","trade, technical, or vocational training":"#EFA1AA","No, not an ethnic minority":"#1FA8C9","Yes, an ethnic minority":"#454E7C","":"#5AC189","Yes":"#FF7F44","No":"#666666","last_yr_income":"#E04355","More":"#A1A6BD","Less":"#ACE1C4","I":"#FEC0A1","expected_earn":"#B2B2B2","Yes: Willing To":"#EFA1AA","No: Not Willing to":"#FDE380","No Answer":"#D3B3DA","In an Office (with Other Developers)":"#9EE5E5","No Preference":"#D1C6BC","From Home":"#1FA8C9"},"color_scheme_domain":["#1FA8C9","#454E7C","#5AC189","#FF7F44","#666666","#E04355","#FCC700","#A868B7","#3CCCCB","#A38F79","#8FD3E4","#A1A6BD","#ACE1C4","#FEC0A1","#B2B2B2","#EFA1AA","#FDE380","#D3B3DA","#9EE5E5","#D1C6BC"],"shared_label_colors":{"Male":"#5ac19e","Female":"#1f86c9","":"#5AC189","Prefer not to say":"#47457c","No Answer":"#e05043","Yes, an ethnic minority":"#666666","No, not an ethnic minority":"#ffa444","age":"#1FA8C9"},"cross_filters_enabled":false,"filter_scopes":{},"chart_configuration":{},"positions":{}}', + '{"timed_refresh_immune_slices":[],"expanded_slices":{},"refresh_frequency":0,"default_filters":"{}","color_scheme":"supersetColors","label_colors":{"0":"#FCC700","1":"#A868B7","15":"#3CCCCB","30":"#A38F79","45":"#8FD3E4","age":"#1FA8C9","Yes,":"#1FA8C9","Female":"#454E7C","Prefer":"#5AC189","No,":"#FF7F44","Male":"#666666","Prefer not to say":"#E04355","Ph.D.":"#FCC700","associate\'s degree":"#A868B7","bachelor\'s degree":"#3CCCCB","high school diploma or equivalent (GED)":"#A38F79","master\'s degree (non-professional)":"#8FD3E4","no high school (secondary school)":"#A1A6BD","professional degree (MBA, MD, JD, etc.)":"#ACE1C4","some college credit, no degree":"#FEC0A1","some high school":"#B2B2B2","trade, technical, or vocational training":"#EFA1AA","No, not an ethnic minority":"#1FA8C9","Yes, an ethnic minority":"#454E7C","":"#5AC189","Yes":"#FF7F44","No":"#666666","last_yr_income":"#E04355","More":"#A1A6BD","Less":"#ACE1C4","I":"#FEC0A1","expected_earn":"#B2B2B2","Yes: Willing To":"#EFA1AA","No: Not Willing to":"#FDE380","No Answer":"#D3B3DA","In an Office (with Other Developers)":"#9EE5E5","No Preference":"#D1C6BC","From Home":"#1FA8C9"},"color_scheme_domain":["#1FA8C9","#454E7C","#5AC189","#FF7F44","#666666","#E04355","#FCC700","#A868B7","#3CCCCB","#A38F79","#8FD3E4","#A1A6BD","#ACE1C4","#FEC0A1","#B2B2B2","#EFA1AA","#FDE380","#D3B3DA","#9EE5E5","#D1C6BC"],"shared_label_colors":["Male", "Female","","Prefer not to say","No Answer","Yes, an ethnic minority","No, not an ethnic minority","age"],"cross_filters_enabled":false,"filter_scopes":{},"chart_configuration":{},"positions":{}}', }, }; diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index d9d095dfb7b2f..63a7a9a737104 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -55,6 +55,7 @@ import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters'; import { safeStringify } from 'src/utils/safeStringify'; import { logEvent } from 'src/logger/actions'; import { LOG_ACTIONS_CONFIRM_OVERWRITE_DASHBOARD_METADATA } from 'src/logger/LogUtils'; +import { isEqual } from 'lodash'; import { UPDATE_COMPONENTS_PARENTS_LIST } from './dashboardLayout'; import { saveChartConfiguration, @@ -68,9 +69,10 @@ import getOverwriteItems from '../util/getOverwriteItems'; import { applyColors, isLabelsColorMapSynced, - getLabelsColorMapEntries, getColorSchemeDomain, getColorNamespace, + getLabelsColorMapEntries, + getFreshSharedLabels, } from '../../utils/colorScheme'; export const SET_UNSAVED_CHANGES = 'SET_UNSAVED_CHANGES'; @@ -224,6 +226,41 @@ export function saveDashboardFinished() { return { type: SAVE_DASHBOARD_FINISHED }; } +export const SET_DASHBOARD_LABELS_COLORMAP_SYNCABLE = + 'SET_DASHBOARD_LABELS_COLORMAP_SYNCABLE'; +export const SET_DASHBOARD_LABELS_COLORMAP_SYNCED = + 'SET_DASHBOARD_LABELS_COLORMAP_SYNCED'; +export const SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCABLE = + 'SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCABLE'; +export const SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCED = + 'SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCED'; + +export function setDashboardLabelsColorMapSync() { + return { type: SET_DASHBOARD_LABELS_COLORMAP_SYNCABLE }; +} + +export function setDashboardLabelsColorMapSynced() { + return { type: SET_DASHBOARD_LABELS_COLORMAP_SYNCED }; +} + +export function setDashboardSharedLabelsColorsSync() { + return { type: SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCABLE }; +} + +export function setDashboardSharedLabelsColorsSynced() { + return { type: SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCED }; +} + +export const setDashboardMetadata = updatedMetadata => async dispatch => { + dispatch( + dashboardInfoChanged({ + metadata: { + ...updatedMetadata, + }, + }), + ); +}; + export function saveDashboardRequest(data, id, saveType) { return (dispatch, getState) => { dispatch({ type: UPDATE_COMPONENTS_PARENTS_LIST }); @@ -254,6 +291,9 @@ export function saveDashboardRequest(data, id, saveType) { const hasId = item => item.id !== undefined; const metadataCrossFiltersEnabled = data.metadata?.cross_filters_enabled; + const colorScheme = data.metadata?.color_scheme; + const customLabelsColor = data.metadata?.label_colors || {}; + const sharedLabelsColor = data.metadata?.shared_label_colors || []; // making sure the data is what the backend expects const cleanedData = { ...data, @@ -270,11 +310,14 @@ export function saveDashboardRequest(data, id, saveType) { metadata: { ...data.metadata, color_namespace: getColorNamespace(data.metadata?.color_namespace), - color_scheme: data.metadata?.color_scheme || '', - color_scheme_domain: data.metadata?.color_scheme_domain || [], + color_scheme: colorScheme || '', + color_scheme_domain: colorScheme + ? getColorSchemeDomain(colorScheme) + : [], expanded_slices: data.metadata?.expanded_slices || {}, - label_colors: data.metadata?.label_colors || {}, - shared_label_colors: data.metadata?.shared_label_colors || {}, + label_colors: customLabelsColor, + shared_label_colors: getFreshSharedLabels(sharedLabelsColor), + map_label_colors: getLabelsColorMapEntries(customLabelsColor), refresh_frequency: data.metadata?.refresh_frequency || 0, timed_refresh_immune_slices: data.metadata?.timed_refresh_immune_slices || [], @@ -324,11 +367,7 @@ export function saveDashboardRequest(data, id, saveType) { // syncing with the backend transformations of the metadata if (updatedDashboard.json_metadata) { const metadata = JSON.parse(updatedDashboard.json_metadata); - dispatch( - dashboardInfoChanged({ - metadata, - }), - ); + dispatch(setDashboardMetadata(metadata)); if (metadata.chart_configuration) { dispatch({ type: SAVE_CHART_CONFIG_COMPLETE, @@ -677,68 +716,261 @@ export function setDatasetsStatus(status) { }; } -const updateDashboardMetadata = async (id, metadata, dispatch) => { - await SupersetClient.put({ +const storeDashboardMetadata = async (id, metadata) => + SupersetClient.put({ endpoint: `/api/v1/dashboard/${id}`, headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ json_metadata: JSON.stringify(metadata) }), }); - dispatch(dashboardInfoChanged({ metadata })); -}; -export const updateDashboardLabelsColor = () => async (dispatch, getState) => { +/** + * + * Persists the label colors maps in the dashboard metadata. + * It runs when outdated color info are detected in stored metadata. + * + * @returns void + */ +export const persistDashboardLabelsColor = () => async (dispatch, getState) => { const { dashboardInfo: { id, metadata }, + dashboardState: { labelsColorMapMustSync, sharedLabelsColorsMustSync }, } = getState(); - const categoricalSchemes = getCategoricalSchemeRegistry(); - const colorScheme = metadata?.color_scheme; - const colorSchemeRegistry = categoricalSchemes.get( - metadata?.color_scheme, - true, - ); - const defaultScheme = categoricalSchemes.defaultKey; - const fallbackScheme = defaultScheme?.toString() || 'supersetColors'; - const colorSchemeDomain = metadata?.color_scheme_domain || []; + if (labelsColorMapMustSync || sharedLabelsColorsMustSync) { + storeDashboardMetadata(id, metadata); + dispatch(setDashboardLabelsColorMapSynced()); + dispatch(setDashboardSharedLabelsColorsSynced()); + } +}; + +/** + * Checks the stored dashboard metadata for inconsistencies. + * Update the current metadata with validated color information. + * It runs only on Dashboard page load. + * + * @param {*} metadata - the stored dashboard metadata + * @returns void + */ +export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => { try { const updatedMetadata = { ...metadata }; - let updatedScheme = metadata?.color_scheme; + const customLabelsColor = metadata.label_colors || {}; + const sharedLabelsColor = metadata.shared_label_colors || []; + let hasChanged = false; + + // backward compatibility of shared_label_colors + const sharedLabels = metadata.shared_label_colors || []; + if (!Array.isArray(sharedLabels) && Object.keys(sharedLabels).length > 0) { + hasChanged = true; + updatedMetadata.shared_label_colors = getFreshSharedLabels( + Object.keys(sharedLabelsColor), + ); + } + // backward compatibility of map_label_colors + const hasMapLabelColors = + Object.keys(metadata.map_label_colors || {}).length > 0; + + let updatedScheme = metadata.color_scheme; + const categoricalSchemes = getCategoricalSchemeRegistry(); + const colorSchemeRegistry = categoricalSchemes.get(updatedScheme, true); + const hasInvalidColorScheme = !!updatedScheme && !colorSchemeRegistry; + + // color scheme might not exist any longer + if (hasInvalidColorScheme) { + const defaultScheme = categoricalSchemes.defaultKey; + const fallbackScheme = defaultScheme?.toString() || 'supersetColors'; + hasChanged = true; - // Color scheme does not exist anymore, fallback to default - if (colorScheme && !colorSchemeRegistry) { updatedScheme = fallbackScheme; updatedMetadata.color_scheme = updatedScheme; - updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme); dispatch(setColorScheme(updatedScheme)); - // must re-apply colors from fresh labels color map - applyColors(updatedMetadata, true); - } - - // stored labels color map and applied might differ - const isMapSynced = isLabelsColorMapSynced(metadata); - if (!isMapSynced) { - // re-apply a fresh labels color map - applyColors(updatedMetadata, true); - // pull and store the just applied labels color map - updatedMetadata.shared_label_colors = getLabelsColorMapEntries(); } // the stored color domain registry and fresh might differ at this point - const freshColorSchemeDomain = getColorSchemeDomain(colorScheme); - const isRegistrySynced = - colorSchemeDomain.toString() !== freshColorSchemeDomain.toString(); - if (colorScheme && !isRegistrySynced) { + const freshColorSchemeDomain = updatedScheme + ? getColorSchemeDomain(updatedScheme) + : []; + const currentColorSchemeDomain = metadata.color_scheme_domain || []; + + if (!isEqual(freshColorSchemeDomain, currentColorSchemeDomain)) { + hasChanged = true; updatedMetadata.color_scheme_domain = freshColorSchemeDomain; } - if ( - (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) || - !isMapSynced - ) { - await updateDashboardMetadata(id, updatedMetadata, dispatch); + // if color scheme is invalid or map is missing, apply a fresh color map + // if valid, apply the stored map to keep consistency across refreshes + const shouldGoFresh = !hasMapLabelColors || hasInvalidColorScheme; + applyColors(updatedMetadata, shouldGoFresh); + + if (shouldGoFresh) { + // a fresh color map has been applied + // needs to be stored for consistency + hasChanged = true; + updatedMetadata.map_label_colors = + getLabelsColorMapEntries(customLabelsColor); + } + + if (hasChanged) { + dispatch(setDashboardMetadata(updatedMetadata)); + dispatch(setDashboardLabelsColorMapSync()); } - } catch (error) { - console.error('Failed to update dashboard color settings:', error); + } catch (e) { + console.error('Failed to update dashboard color on load:', e); } }; + +/** + * + * Ensure that the stored color map matches fresh map. + * + * @param {*} metadata - the dashboard metadata + * @returns void + */ +export const ensureSyncedLabelsColorMap = metadata => (dispatch, getState) => { + const { + dashboardState: { labelsColorMapMustSync }, + } = getState(); + const updatedMetadata = { ...metadata }; + const customLabelsColor = metadata.label_colors || {}; + const isMapSynced = isLabelsColorMapSynced(metadata); + const mustSync = !isMapSynced; + + if (mustSync) { + const freshestColorMapEntries = getLabelsColorMapEntries(customLabelsColor); + updatedMetadata.map_label_colors = freshestColorMapEntries; + dispatch(setDashboardMetadata(updatedMetadata)); + } + + if (mustSync && !labelsColorMapMustSync) { + // prepare to persist the just applied labels color map + dispatch(setDashboardLabelsColorMapSync()); + } + if (!mustSync && labelsColorMapMustSync) { + dispatch(setDashboardLabelsColorMapSynced()); + } +}; + +/** + * + * Ensure that the stored shared labels colors match current. + * + * @param {*} metadata - the dashboard metadata + * @returns void + */ +export const ensureSyncedSharedLabelsColors = + metadata => (dispatch, getState) => { + // using a timeout to let the rendered charts finish processing labels + setTimeout(() => { + const { + dashboardState: { sharedLabelsColorsMustSync }, + } = getState(); + const updatedMetadata = { ...metadata }; + const sharedLabelsColors = metadata.shared_label_colors || []; + const freshLabelsColors = getFreshSharedLabels(sharedLabelsColors); + const isSharedLabelsColorsSynced = isEqual( + sharedLabelsColors, + freshLabelsColors, + ); + + const mustSync = !isSharedLabelsColorsSynced; + + if (mustSync) { + updatedMetadata.shared_label_colors = freshLabelsColors; + dispatch(setDashboardMetadata(updatedMetadata)); + } + + if (mustSync && !sharedLabelsColorsMustSync) { + // prepare to persist the shared labels colors + dispatch(setDashboardSharedLabelsColorsSync()); + } + if (!mustSync && sharedLabelsColorsMustSync) { + dispatch(setDashboardSharedLabelsColorsSynced()); + } + }, 500); + }; + +/** + * + * Updates the color map with new labels and colors as they appear. + * + * @param {*} renderedChartIds - the charts that have finished rendering + * @returns void + */ +export const updateDashboardLabelsColor = + renderedChartIds => (dispatch, getState) => { + try { + const { + dashboardInfo: { metadata }, + charts, + } = getState(); + const colorScheme = metadata.color_scheme; + const labelsColorMapInstance = getLabelsColorMap(); + const fullLabelsColors = metadata.map_label_colors || {}; + const sharedLabelsColors = metadata.shared_label_colors || []; + const customLabelsColors = metadata.label_colors || {}; + const updatedMetadata = { ...metadata }; + + // for dashboards with no color scheme, the charts should always use their individual schemes + // this logic looks for unique labels (not shared across multiple charts) of each rendered chart + // it applies a new color to those unique labels when the applied scheme is not up to date + // while leaving shared label colors and custom label colors intact for color consistency + const shouldReset = []; + if (renderedChartIds.length > 0) { + const sharedLabelsSet = new Set(sharedLabelsColors); + renderedChartIds.forEach(id => { + const chart = charts[id]; + const formData = chart.form_data || chart.latestQueryFormData; + // ensure charts have their original color scheme always available + labelsColorMapInstance.setOwnColorScheme( + formData.slice_id, + formData.color_scheme, + ); + + // if dashboard has a scheme, charts should ignore individual schemes + // thus following logic is inapplicable if a dashboard color scheme exists + if (colorScheme) return; + + const chartColorScheme = formData.color_scheme; + const currentChartConfig = labelsColorMapInstance.chartsLabelsMap.get( + formData.slice_id, + ); + const currentChartLabels = currentChartConfig?.labels || []; + const uniqueChartLabels = currentChartLabels.filter( + l => + !sharedLabelsSet.has(l) && !customLabelsColors.hasOwnProperty(l), + ); + + // Map unique labels to colors + const uniqueChartLabelsColor = new Set( + uniqueChartLabels.map(l => fullLabelsColors[l]).filter(Boolean), + ); + + const expectedColorsForChartScheme = new Set( + getColorSchemeDomain(chartColorScheme), + ); + + // Check if any unique label color is not in the expected colors set + const shouldResetColors = [...uniqueChartLabelsColor].some( + color => !expectedColorsForChartScheme.has(color), + ); + + // Only push uniqueChartLabels if they require resetting + if (shouldResetColors) shouldReset.push(...uniqueChartLabels); + }); + } + + // an existing map is available, use mrge option + // to only apply colors to newly found labels + const shouldGoFresh = shouldReset.length > 0 ? shouldReset : false; + const shouldMerge = !shouldGoFresh; + // re-apply the color map first to get fresh maps accordingly + applyColors(updatedMetadata, shouldGoFresh, shouldMerge); + // new data may have appeared in the map (data changes) + // or new slices may have appeared while changing tabs + dispatch(ensureSyncedLabelsColorMap(updatedMetadata)); + dispatch(ensureSyncedSharedLabelsColors(updatedMetadata)); + } catch (e) { + console.error('Failed to update colors for new charts and labels:', e); + } + }; diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index 24d61b7d12e19..7da2064f96305 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -36,6 +36,8 @@ import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants'; fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {}); +fetchMock.put('glob:*/api/v1/dashboard/*', {}); + jest.mock('src/dashboard/actions/dashboardState', () => ({ ...jest.requireActual('src/dashboard/actions/dashboardState'), fetchFaveStar: jest.fn(), diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx index 7c27a9f1f7888..36dedb68c7c3e 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx @@ -18,7 +18,7 @@ */ // ParentSize uses resize observer so the dashboard will update size // when its container size changes, due to e.g., builder side panel opening -import { FC, useEffect, useMemo, useRef } from 'react'; +import { FC, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { Filter, @@ -43,12 +43,12 @@ import { import { getChartIdsInFilterScope } from 'src/dashboard/util/getChartIdsInFilterScope'; import findTabIndexByComponentId from 'src/dashboard/util/findTabIndexByComponentId'; import { setInScopeStatusOfFilters } from 'src/dashboard/actions/nativeFilters'; -import { updateDashboardLabelsColor } from 'src/dashboard/actions/dashboardState'; import { - applyColors, - getColorNamespace, - resetColors, -} from 'src/utils/colorScheme'; + applyDashboardLabelsColorOnLoad, + updateDashboardLabelsColor, + persistDashboardLabelsColor, +} from 'src/dashboard/actions/dashboardState'; +import { getColorNamespace, resetColors } from 'src/utils/colorScheme'; import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils'; import { findTabsWithChartsInScope } from '../nativeFilters/utils'; import { getRootLevelTabsComponent } from './utils'; @@ -88,6 +88,14 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { const chartIds = useSelector(state => Object.values(state.charts).map(chart => chart.id), ); + const renderedChartIds = useSelector(state => + Object.values(state.charts) + .filter(chart => chart.chartStatus === 'rendered') + .map(chart => chart.id), + ); + const [dashboardLabelsColorInitiated, setDashboardLabelsColorInitiated] = + useState(false); + const prevRenderedChartIds = useRef([]); const prevTabIndexRef = useRef(); const tabIndex = useMemo(() => { @@ -140,28 +148,65 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { const activeKey = min === 0 ? DASHBOARD_GRID_ID : min.toString(); const TOP_OF_PAGE_RANGE = 220; + const onBeforeUnload = useCallback(() => { + dispatch(persistDashboardLabelsColor()); + resetColors(getColorNamespace(dashboardInfo?.metadata?.color_namespace)); + prevRenderedChartIds.current = []; + }, [dashboardInfo?.metadata?.color_namespace, dispatch]); + useEffect(() => { - // verify freshness of color map on tab change - // and when loading for first time - setTimeout(() => { - dispatch(updateDashboardLabelsColor()); - }, 500); - }, [directPathToChild, dispatch]); + // verify freshness of color map + // when charts render to catch new labels + const numRenderedCharts = renderedChartIds.length; + + if ( + dashboardLabelsColorInitiated && + dashboardInfo?.id && + numRenderedCharts > 0 && + prevRenderedChartIds.current.length < numRenderedCharts + ) { + const newRenderedChartIds = renderedChartIds.filter( + id => !prevRenderedChartIds.current.includes(id), + ); + prevRenderedChartIds.current = renderedChartIds; + dispatch(updateDashboardLabelsColor(newRenderedChartIds)); + } + }, [ + dashboardInfo?.id, + renderedChartIds, + dispatch, + dashboardLabelsColorInitiated, + ]); useEffect(() => { const labelsColorMap = getLabelsColorMap(); - const colorNamespace = getColorNamespace( - dashboardInfo?.metadata?.color_namespace, - ); labelsColorMap.source = LabelsColorMapSource.Dashboard; - // apply labels color as dictated by stored metadata - applyColors(dashboardInfo.metadata); + + if (dashboardInfo?.id && !dashboardLabelsColorInitiated) { + // apply labels color as dictated by stored metadata (if any) + setDashboardLabelsColorInitiated(true); + dispatch(applyDashboardLabelsColorOnLoad(dashboardInfo.metadata)); + } return () => { - resetColors(getColorNamespace(colorNamespace)); + onBeforeUnload(); }; + // eslint-disable-next-line react-hooks/exhaustive-deps - }, [dashboardInfo.id, dispatch]); + }, [dashboardInfo?.id, dispatch]); + + useEffect(() => { + // 'beforeunload' event interferes with Cypress data cleanup process. + // This code prevents 'beforeunload' from triggering in Cypress tests, + // as it is not required for end-to-end testing scenarios. + if (!(window as any).Cypress) { + window.addEventListener('beforeunload', onBeforeUnload); + } + + return () => { + window.removeEventListener('beforeunload', onBeforeUnload); + }; + }, [onBeforeUnload]); return (
diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 39dcff8d4c0d0..c0fc97838cb83 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -461,7 +461,6 @@ class Header extends PureComponent { customCss, colorNamespace, dataMask, - setColorScheme, setUnsavedChanges, colorScheme, onUndo, @@ -501,7 +500,6 @@ class Header extends PureComponent { const handleOnPropertiesChange = updates => { const { dashboardInfoChanged, dashboardTitleChanged } = this.props; - setColorScheme(updates.colorScheme); dashboardInfoChanged({ slug: updates.slug, metadata: JSON.parse(updates.jsonMetadata || '{}'), diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 0bfa1706d577a..998e8540dcef3 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { omit } from 'lodash'; import { Input } from 'src/components/Input'; import { FormItem } from 'src/components/Form'; @@ -44,9 +44,19 @@ import withToasts from 'src/components/MessageToasts/withToasts'; import TagType from 'src/types/TagType'; import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags'; import { loadTags } from 'src/components/Tags/utils'; -import { applyColors, getColorNamespace } from 'src/utils/colorScheme'; +import { + applyColors, + getColorNamespace, + getLabelsColorMapEntries, +} from 'src/utils/colorScheme'; import getOwnerName from 'src/utils/getOwnerName'; import Owner from 'src/types/Owner'; +import { useDispatch } from 'react-redux'; +import { + setColorScheme, + setDashboardMetadata, +} from 'src/dashboard/actions/dashboardState'; +import { areObjectsEqual } from 'src/reduxUtils'; const StyledFormItem = styled(FormItem)` margin-bottom: 0; @@ -84,6 +94,7 @@ type DashboardInfo = { certifiedBy: string; certificationDetails: string; isManagedExternally: boolean; + metadata: Record; }; const PropertiesModal = ({ @@ -98,10 +109,11 @@ const PropertiesModal = ({ onSubmit = () => {}, show = false, }: PropertiesModalProps) => { + const dispatch = useDispatch(); const [form] = AntdForm.useForm(); const [isLoading, setIsLoading] = useState(false); const [isAdvancedOpen, setIsAdvancedOpen] = useState(false); - const [colorScheme, setColorScheme] = useState(currentColorScheme); + const [colorScheme, setCurrentColorScheme] = useState(currentColorScheme); const [jsonMetadata, setJsonMetadata] = useState(''); const [dashboardInfo, setDashboardInfo] = useState(); const [owners, setOwners] = useState([]); @@ -109,6 +121,7 @@ const PropertiesModal = ({ const saveLabel = onlyApply ? t('Apply') : t('Save'); const [tags, setTags] = useState([]); const categoricalSchemeRegistry = getCategoricalSchemeRegistry(); + const originalDashboardMetadata = useRef>({}); const tagsAsSelectValues = useMemo(() => { const selectTags = tags.map((tag: { id: number; name: string }) => ({ @@ -182,21 +195,24 @@ const PropertiesModal = ({ certifiedBy: certified_by || '', certificationDetails: certification_details || '', isManagedExternally: is_managed_externally || false, + metadata, }; form.setFieldsValue(dashboardInfo); setDashboardInfo(dashboardInfo); setOwners(owners); setRoles(roles); - setColorScheme(metadata.color_scheme); + setCurrentColorScheme(metadata.color_scheme); const metaDataCopy = omit(metadata, [ 'positions', 'shared_label_colors', + 'map_label_colors', 'color_scheme_domain', ]); setJsonMetadata(metaDataCopy ? jsonStringify(metaDataCopy) : ''); + originalDashboardMetadata.current = metadata; }, [form], ); @@ -269,6 +285,8 @@ const PropertiesModal = ({ return parsedRoles; }; + const handleOnCancel = () => onHide(); + const onColorSchemeChange = ( colorScheme = '', { updateMetadata = true } = {}, @@ -287,20 +305,21 @@ const PropertiesModal = ({ throw new Error('A valid color scheme is required'); } + jsonMetadataObj.color_scheme = colorScheme; + jsonMetadataObj.label_colors = jsonMetadataObj.label_colors || {}; + + setCurrentColorScheme(colorScheme); + dispatch(setColorScheme(colorScheme)); + // update metadata to match selection if (updateMetadata) { - jsonMetadataObj.color_scheme = colorScheme; - jsonMetadataObj.label_colors = jsonMetadataObj.label_colors || {}; - setJsonMetadata(jsonStringify(jsonMetadataObj)); } - setColorScheme(colorScheme); }; const onFinish = () => { const { title, slug, certifiedBy, certificationDetails } = form.getFieldsValue(); - let currentColorScheme = colorScheme; let currentJsonMetadata = jsonMetadata; // validate currentJsonMetadata @@ -318,29 +337,48 @@ const PropertiesModal = ({ return; } - const copyMetadata = { ...metadata }; const colorNamespace = getColorNamespace(metadata?.color_namespace); - // color scheme in json metadata has precedence over selection - currentColorScheme = metadata?.color_scheme || colorScheme; - - // remove information from user facing input - if (metadata?.shared_label_colors) { - delete metadata.shared_label_colors; - } - if (metadata?.color_scheme_domain) { - delete metadata.color_scheme_domain; - } - - // only apply colors, the user has not saved yet - applyColors(copyMetadata, true); + const updatedColorScheme = metadata?.color_scheme || colorScheme; + const shouldGoFresh = + updatedColorScheme !== originalDashboardMetadata.current.color_scheme; + const shouldResetCustomLabels = !areObjectsEqual( + originalDashboardMetadata.current.label_colors || {}, + metadata?.label_colors || {}, + ); + const currentCustomLabels = Object.keys(metadata?.label_colors || {}); + const prevCustomLabels = Object.keys( + originalDashboardMetadata.current.label_colors || {}, + ); + const resettableCustomLabels = + currentCustomLabels.length > 0 ? currentCustomLabels : prevCustomLabels; + const freshCustomLabels = + shouldResetCustomLabels && resettableCustomLabels.length > 0 + ? resettableCustomLabels + : false; + const jsonMetadataObj = getJsonMetadata(); + const customLabelColors = jsonMetadataObj.label_colors || {}; + const updatedDashboardMetadata = { + ...originalDashboardMetadata.current, + label_colors: customLabelColors, + color_scheme: updatedColorScheme, + }; - currentJsonMetadata = jsonStringify(metadata); + originalDashboardMetadata.current = updatedDashboardMetadata; + applyColors(updatedDashboardMetadata, shouldGoFresh || freshCustomLabels); + dispatch( + setDashboardMetadata({ + ...updatedDashboardMetadata, + map_label_colors: getLabelsColorMapEntries(customLabelColors), + }), + ); - onColorSchemeChange(currentColorScheme, { + onColorSchemeChange(updatedColorScheme, { updateMetadata: false, }); + currentJsonMetadata = jsonStringify(metadata); + const moreOnSubmitProps: { roles?: Roles } = {}; const morePutProps: { roles?: number[]; tags?: (number | undefined)[] } = {}; @@ -557,14 +595,14 @@ const PropertiesModal = ({ return (