From 3e7dc937a54f6b2a5194342324a0f0c5e7475ffd Mon Sep 17 00:00:00 2001 From: Igor Dykhta Date: Mon, 23 Dec 2024 12:52:26 +0200 Subject: [PATCH] [fix] custom palette issues (#2850) * 1 update fix of custom palette issues 2 restrict custom steps to 20, hide + button if needed 3 update scale function to handle some special cases: not enough k groups * add name|type|category to updateCustomPalette if customBreaks * add test cases for redesigned color range feature * update test cases to increase coverage Signed-off-by: Ihor Dykhta Co-authored-by: Ilya Boyandin --- .../side-panel/layer-panel/custom-palette.tsx | 7 +- src/layers/src/base-layer.ts | 41 ++- src/utils/src/color-utils.ts | 41 ++- src/utils/src/data-scale-utils.ts | 9 +- src/utils/src/index.ts | 3 +- .../channel-by-value-selctor-test.js | 6 +- test/node/reducers/vis-state-test.js | 264 +++++++++++++++++- 7 files changed, 342 insertions(+), 29 deletions(-) diff --git a/src/components/src/side-panel/layer-panel/custom-palette.tsx b/src/components/src/side-panel/layer-panel/custom-palette.tsx index 3cf08228a9..c71149febe 100644 --- a/src/components/src/side-panel/layer-panel/custom-palette.tsx +++ b/src/components/src/side-panel/layer-panel/custom-palette.tsx @@ -72,6 +72,7 @@ export type CustomPaletteInputProps = { inputColorHex: (index: number, v: HexColor) => void; editColorMapValue: EditColorMapFunc; actionIcons?: ActionIcons; + disableAppend?: boolean; disableDelete?: boolean; onDelete: (index: number) => void; onAdd: (index: number) => void; @@ -353,6 +354,7 @@ export const CustomPaletteInput: React.FC = ({ inputColorHex, editColorMapValue, actionIcons = defaultActionIcons, + disableAppend, disableDelete, onDelete, onAdd, @@ -394,7 +396,9 @@ export const CustomPaletteInput: React.FC = ({ ) : null}
- + {!disableAppend ? ( + + ) : null} {!disableDelete ? ( ) : null} @@ -555,6 +559,7 @@ function CustomPaletteFactory(): React.FC { isSorting={isSorting} color={color} inputColorHex={inputColorHex} + disableAppend={colors.length >= 20} disableDelete={colors.length <= 2} actionIcons={actionIcons} onAdd={onAdd} diff --git a/src/layers/src/base-layer.ts b/src/layers/src/base-layer.ts index 1ed9964e8b..21f2e9dca6 100644 --- a/src/layers/src/base-layer.ts +++ b/src/layers/src/base-layer.ts @@ -65,7 +65,11 @@ import { RGBColor, ValueOf } from '@kepler.gl/types'; -import {getScaleFunction, initializeLayerColorMap} from '@kepler.gl/utils'; +import { + getScaleFunction, + initializeLayerColorMap, + updateCustomColorRangeByColorUI +} from '@kepler.gl/utils'; import memoize from 'lodash.memoize'; import {isDomainQuantile, getDomainStepsbyZoom, getThresholdsFromQuantiles} from '@kepler.gl/utils'; @@ -914,6 +918,11 @@ class Layer { Console.warn(`updateColorUI: Can't find visual channel which range is ${prop}`); return; } + // add name|type|category to updateCustomPalette if customBreaks, so that + // colors will not be override as well when inverse palette with custom break + customPalette.name = DEFAULT_CUSTOM_PALETTE.name; + customPalette.type = DEFAULT_CUSTOM_PALETTE.type; + customPalette.category = DEFAULT_CUSTOM_PALETTE.category; // initiate colorMap from current scale customPalette.colorMap = initializeLayerColorMap(this, visualChannels[channelKey]); } else if (newConfig.colorRangeConfig.custom) { @@ -940,9 +949,17 @@ class Layer { * @param {*} prop */ updateColorUIByColorRange(newConfig, prop) { - if (typeof newConfig.showDropdown !== 'number') return; - const {colorUI, visConfig} = this.config; + + // when custom palette adds/removes step, the number in "Steps" input control + // should be updated as well + const isCustom = newConfig.customPalette?.category === 'Custom'; + const customStepsChanged = isCustom + ? newConfig.customPalette.colors.length !== visConfig[prop].colors.length + : false; + + if (typeof newConfig.showDropdown !== 'number' && !customStepsChanged) return; + this.updateLayerConfig({ colorUI: { ...colorUI, @@ -950,7 +967,9 @@ class Layer { ...colorUI[prop], colorRangeConfig: { ...colorUI[prop].colorRangeConfig, - steps: visConfig[prop].colors.length, + steps: customStepsChanged + ? colorUI[prop].customPalette.colors.length + : visConfig[prop].colors.length, reversed: Boolean(visConfig[prop].reversed) } } @@ -972,10 +991,16 @@ class Layer { const {colorUI, visConfig} = this.config; - const update = updateColorRangeByMatchingPalette( - visConfig[prop], - colorUI[prop].colorRangeConfig - ); + // for custom palette, one can only 'reverse' the colors in custom palette. + // changing 'steps', 'colorBindSafe', 'type' should fall back to predefined palette. + const isCustomColorReversed = + visConfig.colorRange.category === 'Custom' && + newConfig.colorRangeConfig && + Object.prototype.hasOwnProperty.call(newConfig.colorRangeConfig, 'reversed'); + + const update = isCustomColorReversed + ? updateCustomColorRangeByColorUI(visConfig[prop], colorUI[prop].colorRangeConfig) + : updateColorRangeByMatchingPalette(visConfig[prop], colorUI[prop].colorRangeConfig); if (update) { this.updateLayerVisConfig({[prop]: update}); diff --git a/src/utils/src/color-utils.ts b/src/utils/src/color-utils.ts index 26bedae26b..6436ae827a 100644 --- a/src/utils/src/color-utils.ts +++ b/src/utils/src/color-utils.ts @@ -365,22 +365,24 @@ export function updateColorRangeByMatchingPalette( } /** - * Update color range after selecting a palette from color range selectoer - * Copy over colorMap and colorLegends + * Update custom palette when reverse the colors in custom palette, since changing 'steps', + * 'colorBindSafe', 'type' should fall back to predefined palette. */ -export function updateColorRangeBySelectedPalette( +export function updateCustomColorRangeByColorUI( oldColorRange: ColorRange, - colorPalette: ColorPalette, - colorConfig: { - reversed: boolean; - steps: number; - } + colorConfig: ColorRangeConfig ): ColorRange { - // const {reversed} = colorConfig; + const {reversed} = colorConfig; + const colors = oldColorRange.colors; + // for custom palette, one can only 'reverse' the colors in custom palette. + colors.reverse(); const colorRange = { - ...colorPaletteToColorRange(colorPalette, colorConfig), - // ...(reversed ? {reversed} : {}), + name: oldColorRange.name, + type: oldColorRange.type, + category: oldColorRange.category, + colors, + ...(reversed ? {reversed} : {}), ...(oldColorRange.colorMap ? {colorMap: oldColorRange.colorMap} : {}), ...(oldColorRange.colorLegends ? {colorLegends: oldColorRange.colorLegends} : {}) }; @@ -388,6 +390,23 @@ export function updateColorRangeBySelectedPalette( return replaceColorsInColorRange(colorRange, colorRange.colors); } +/** + * Update color range after selecting a palette from color range selectoer + * Copy over colorMap and colorLegends + */ + export function updateColorRangeBySelectedPalette(oldColorRange, colorPalette, colorConfig) { + const {colors: newColors, ...newColorRange} = colorPaletteToColorRange(colorPalette, colorConfig); + + const colorRange = { + colors: oldColorRange.colors, + ...newColorRange, + ...(oldColorRange.colorMap ? {colorMap: oldColorRange.colorMap} : {}), + ...(oldColorRange.colorLegends ? {colorLegends: oldColorRange.colorLegends} : {}) + }; + + return replaceColorsInColorRange(colorRange, newColors); +} + const UberNameRegex = new RegExp(/^([A-Za-z ])+/g); const ColorBrewerRegex = new RegExp(/^ColorBrewer ([A-Za-z1-9])+/g); diff --git a/src/utils/src/data-scale-utils.ts b/src/utils/src/data-scale-utils.ts index 78f9310da1..6221c3d263 100644 --- a/src/utils/src/data-scale-utils.ts +++ b/src/utils/src/data-scale-utils.ts @@ -218,7 +218,12 @@ export function getQuantLegends(scale: D3ScaleFunction, labelFormat: LabelFormat } const labels = scale.scaleType === 'threshold' || scale.scaleType === 'custom' - ? getThresholdLabels(scale, customScaleLabelFormat) + ? getThresholdLabels( + scale, + scale.scaleType === 'custom' + ? customScaleLabelFormat + : n => (n ? formatNumber(n) : 'no value') + ) : getScaleLabels(scale, labelFormat); const data = scale.range(); @@ -256,7 +261,7 @@ export function getQuantLabelFormat(domain, fieldType) { ? getTimeLabelFormat(domain) : !fieldType ? defaultFormat - : n => formatNumber(n, fieldType); + : n => (n ? formatNumber(n, fieldType) : 'no value'); } /** diff --git a/src/utils/src/index.ts b/src/utils/src/index.ts index a487a88aea..4f3cec33e2 100644 --- a/src/utils/src/index.ts +++ b/src/utils/src/index.ts @@ -22,7 +22,8 @@ export { paletteIsSteps, paletteIsType, paletteIsColorBlindSafe, - updateColorRangeByMatchingPalette + updateColorRangeByMatchingPalette, + updateCustomColorRangeByColorUI } from './color-utils'; export {errorNotification} from './notifications-utils'; diff --git a/test/browser/components/side-panel/channel-by-value-selctor-test.js b/test/browser/components/side-panel/channel-by-value-selctor-test.js index 8b80f195bc..b3243724de 100644 --- a/test/browser/components/side-panel/channel-by-value-selctor-test.js +++ b/test/browser/components/side-panel/channel-by-value-selctor-test.js @@ -44,9 +44,9 @@ function nop() { const ExpectedCustomPalette = { customPalette: { - name: 'Uber Viz Sequential', - type: 'sequential', - category: 'Uber', + name: 'color.customPalette', + type: 'custom', + category: 'Custom', colors: expectedColorRangeInLayer.colors, colorMap: [ [3032, expectedColorRangeInLayer.colors[0]], diff --git a/test/node/reducers/vis-state-test.js b/test/node/reducers/vis-state-test.js index 7c444f0abc..97f90bcf05 100644 --- a/test/node/reducers/vis-state-test.js +++ b/test/node/reducers/vis-state-test.js @@ -4715,15 +4715,17 @@ test('#visStateReducer -> LAYER_COLOR_UI_CHANGE. custom breaks', t => { }) ); + // Explain: with redesigned color range using chormajs/d3, create customBreaks + // will lead to customPalette, which won't be overrided by predefined colorPalette const expectedColorUI = { color: DEFAULT_COLOR_UI, strokeColorRange: DEFAULT_COLOR_UI, colorRange: { ...DEFAULT_COLOR_UI, customPalette: { - name: 'Uber Viz Sequential', - type: 'sequential', - category: 'Uber', + name: 'color.customPalette', + type: 'custom', + category: 'Custom', colors: ['#00939C', '#6BB5B9', '#AAD7D9', '#E6FAFA'], colorMap: [ ['driver_analytics', '#00939C'], @@ -6838,3 +6840,259 @@ test('#VisStateUpdater -> removeEffect', t => { t.end(); }); + +test('#visStateReducer -> LAYER_COLOR_UI_CHANGE. enable custom palette', t => { + const initialState = CloneDeep(StateWFilesFiltersLayerColor.visState); + const pointLayer = initialState.layers[0]; + + const nextState = reducer( + initialState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {custom: true} + }) + ); + + const expectedColorUI = { + color: DEFAULT_COLOR_UI, + strokeColorRange: DEFAULT_COLOR_UI, + colorRange: { + ...DEFAULT_COLOR_UI, + customPalette: { + name: 'color.customPalette', + type: 'custom', + category: 'Custom', + colors: ['#00939C', '#6BB5B9', '#AAD7D9', '#E6FAFA'] + }, + colorRangeConfig: { + type: 'all', + colorBlindSafe: false, + steps: 6, + reversed: false, + custom: true, + customBreaks: false + } + } + }; + + t.deepEqual( + nextState.layers[0].config.colorUI, + expectedColorUI, + 'should set customPalette when one clicks Custom Palette and Confirm button' + ); + + t.end(); +}); + +test('#visStateReducer -> LAYER_COLOR_UI_CHANGE. custom palette - delete color item', t => { + const initialState = CloneDeep(StateWFilesFiltersLayerColor.visState); + const pointLayer = initialState.layers[0]; + + const customPalette = { + customPalette: { + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'], + name: 'color.customPalette', + type: 'custom', + category: 'Custom' + } + }; + + const nextState = reducer( + initialState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {custom: true} + }) + ); + + const deleteState = reducer( + nextState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', customPalette) + ); + + const expectedColorUI = { + color: DEFAULT_COLOR_UI, + strokeColorRange: DEFAULT_COLOR_UI, + colorRange: { + ...DEFAULT_COLOR_UI, + customPalette: { + name: 'color.customPalette', + type: 'custom', + category: 'Custom', + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'] + }, + colorRangeConfig: { + type: 'all', + colorBlindSafe: false, + steps: 3, + reversed: false, + custom: true, + customBreaks: false + } + } + }; + + t.deepEqual( + deleteState.layers[0].config.colorUI, + expectedColorUI, + 'should set customPalette when one deletes a color item from Custom Palette' + ); + + let confirmState = reducer( + deleteState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {custom: false} + }) + ); + + // simulate Confirm button action + confirmState = reducer( + confirmState, + VisStateActions.layerVisConfigChange(pointLayer, { + colorRange: { + name: 'color.customPalette', + type: 'custom', + category: 'Custom', + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'] + } + }) + ); + + const reverseState = reducer( + confirmState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {reversed: true} + }) + ); + + const expectedReversedColorUI = { + ...expectedColorUI, + colorRange: { + ...expectedColorUI.colorRange, + colorRangeConfig: { + type: 'all', + colorBlindSafe: false, + steps: 3, + reversed: true, + custom: false, + customBreaks: false + } + } + }; + + t.deepEqual( + reverseState.layers[0].config.colorUI, + expectedReversedColorUI, + 'should set customPalette when one deletes a color item from Custom Palette and reverse it' + ); + + const expectedReversedVisConfigColorRange = { + category: 'Custom', + colors: ['#E6FAFA', '#AAD7D9', '#6BB5B9'], + name: 'color.customPalette', + reversed: true, + type: 'custom' + }; + + t.deepEqual( + reverseState.layers[0].config.visConfig.colorRange, + expectedReversedVisConfigColorRange, + 'should set visConfig.ColorRange when one deletes a color item from Custom Palette and reverse it' + ); + + t.end(); +}); + +test('#visStateReducer -> LAYER_COLOR_UI_CHANGE. custom palette - select new steps', t => { + const initialState = CloneDeep(StateWFilesFiltersLayerColor.visState); + const pointLayer = initialState.layers[0]; + + const customPalette = { + customPalette: { + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'], + name: 'color.customPalette', + type: 'custom', + category: 'Custom' + } + }; + + const nextState = reducer( + initialState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {custom: true} + }) + ); + + const deleteState = reducer( + nextState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', customPalette) + ); + + let confirmState = reducer( + deleteState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {custom: false} + }) + ); + + // simulate Confirm button action + confirmState = reducer( + confirmState, + VisStateActions.layerVisConfigChange(pointLayer, { + colorRange: { + name: 'color.customPalette', + type: 'custom', + category: 'Custom', + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'] + } + }) + ); + + const stepsState = reducer( + confirmState, + VisStateActions.layerColorUIChange(pointLayer, 'colorRange', { + colorRangeConfig: {steps: 4} + }) + ); + + const expectedColorUI = { + color: DEFAULT_COLOR_UI, + strokeColorRange: DEFAULT_COLOR_UI, + colorRange: { + ...DEFAULT_COLOR_UI, + customPalette: { + name: 'color.customPalette', + type: 'custom', + category: 'Custom', + colors: ['#6BB5B9', '#AAD7D9', '#E6FAFA'] + }, + colorRangeConfig: { + type: 'all', + colorBlindSafe: false, + steps: 4, + reversed: false, + custom: false, + customBreaks: false + } + } + }; + + const expectedVisConfigColorRange = { + colors: ['#00939C', '#8BC6C9', '#EB9373', '#C22E00'], + name: 'Uber Viz Diverging', + type: 'diverging', + category: 'Uber' + }; + + t.deepEqual( + stepsState.layers[0].config.colorUI, + expectedColorUI, + 'should set correct step in colorRangeConfig when one deletes a color item from Custom Palette and select new steps' + ); + + t.deepEqual( + stepsState.layers[0].config.visConfig.colorRange, + expectedVisConfigColorRange, + 'should set predefined palette in visConfig.colorRange when one deletes a color item from Custom Palette and select new steps' + ); + + t.end(); +});