From 21b4752dba84d8c2ebaa29ec4a65703ea416d60d Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Fri, 3 Sep 2021 15:57:57 +0200 Subject: [PATCH] [Lens] Fix transition to custom palette inconsistency when in number mode (#110852) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../public/services/palettes/helpers.ts | 5 +- .../coloring/palette_configuration.test.tsx | 182 ++++++++++++------ .../coloring/palette_configuration.tsx | 36 ++-- .../shared_components/coloring/utils.test.ts | 73 +++++++ .../shared_components/coloring/utils.ts | 5 +- x-pack/test/functional/apps/lens/table.ts | 21 ++ 6 files changed, 246 insertions(+), 76 deletions(-) diff --git a/src/plugins/charts/public/services/palettes/helpers.ts b/src/plugins/charts/public/services/palettes/helpers.ts index d4b1e98f94cc8..bd1f8350ba9f3 100644 --- a/src/plugins/charts/public/services/palettes/helpers.ts +++ b/src/plugins/charts/public/services/palettes/helpers.ts @@ -70,7 +70,10 @@ export function workoutColorForValue( const comparisonFn = (v: number, threshold: number) => v - threshold; // if steps are defined consider the specific rangeMax/Min as data boundaries - const maxRange = stops.length ? rangeMax : dataRangeArguments[1]; + // as of max reduce its value by 1/colors.length for correct continuity checks + const maxRange = stops.length + ? rangeMax + : dataRangeArguments[1] - (dataRangeArguments[1] - dataRangeArguments[0]) / colors.length; const minRange = stops.length ? rangeMin : dataRangeArguments[0]; // in case of shorter rangers, extends the steps on the sides to cover the whole set diff --git a/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.test.tsx b/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.test.tsx index ad1755bdbe85c..cda891871168e 100644 --- a/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.test.tsx +++ b/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.test.tsx @@ -6,7 +6,7 @@ */ import React from 'react'; -import { EuiColorPalettePickerPaletteProps } from '@elastic/eui'; +import { EuiButtonGroup, EuiColorPalettePickerPaletteProps } from '@elastic/eui'; import { mountWithIntl } from '@kbn/test/jest'; import { chartPluginMock } from 'src/plugins/charts/public/mocks'; import type { PaletteOutput, PaletteRegistry } from 'src/plugins/charts/public'; @@ -14,6 +14,7 @@ import { ReactWrapper } from 'enzyme'; import type { CustomPaletteParams } from '../../../common'; import { applyPaletteParams } from './utils'; import { CustomizablePalette } from './palette_configuration'; +import { act } from 'react-dom/test-utils'; // mocking random id generator function jest.mock('@elastic/eui', () => { @@ -128,71 +129,136 @@ describe('palette panel', () => { }); }); - describe('reverse option', () => { - beforeEach(() => { - props = { - activePalette: { type: 'palette', name: 'positive' }, - palettes: paletteRegistry, - setPalette: jest.fn(), - dataBounds: { min: 0, max: 100 }, - }; - }); + it('should rewrite the min/max range values on palette change', () => { + const instance = mountWithIntl(); + + changePaletteIn(instance, 'custom'); - function toggleReverse(instance: ReactWrapper, checked: boolean) { - return instance - .find('[data-test-subj="lnsPalettePanel_dynamicColoring_reverse"]') - .first() - .prop('onClick')!({} as React.MouseEvent); - } - - it('should reverse the colorStops on click', () => { - const instance = mountWithIntl(); - - toggleReverse(instance, true); - - expect(props.setPalette).toHaveBeenCalledWith( - expect.objectContaining({ - params: expect.objectContaining({ - reverse: true, - }), - }) - ); + expect(props.setPalette).toHaveBeenCalledWith({ + type: 'palette', + name: 'custom', + params: expect.objectContaining({ + rangeMin: 0, + rangeMax: 50, + }), }); }); + }); + + describe('reverse option', () => { + beforeEach(() => { + props = { + activePalette: { type: 'palette', name: 'positive' }, + palettes: paletteRegistry, + setPalette: jest.fn(), + dataBounds: { min: 0, max: 100 }, + }; + }); + + function toggleReverse(instance: ReactWrapper, checked: boolean) { + return instance + .find('[data-test-subj="lnsPalettePanel_dynamicColoring_reverse"]') + .first() + .prop('onClick')!({} as React.MouseEvent); + } + + it('should reverse the colorStops on click', () => { + const instance = mountWithIntl(); + + toggleReverse(instance, true); + + expect(props.setPalette).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ + reverse: true, + }), + }) + ); + }); + }); + + describe('percentage / number modes', () => { + beforeEach(() => { + props = { + activePalette: { type: 'palette', name: 'positive' }, + palettes: paletteRegistry, + setPalette: jest.fn(), + dataBounds: { min: 5, max: 200 }, + }; + }); - describe('custom stops', () => { - beforeEach(() => { - props = { - activePalette: { type: 'palette', name: 'positive' }, - palettes: paletteRegistry, - setPalette: jest.fn(), - dataBounds: { min: 0, max: 100 }, - }; + it('should switch mode and range boundaries on click', () => { + const instance = mountWithIntl(); + act(() => { + instance + .find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_range_groups"]') + .find(EuiButtonGroup) + .prop('onChange')!('number'); }); - it('should be visible for predefined palettes', () => { - const instance = mountWithIntl(); - expect( - instance.find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_stops"]').exists() - ).toEqual(true); + + act(() => { + instance + .find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_range_groups"]') + .find(EuiButtonGroup) + .prop('onChange')!('percent'); }); - it('should be visible for custom palettes', () => { - const instance = mountWithIntl( - { + beforeEach(() => { + props = { + activePalette: { type: 'palette', name: 'positive' }, + palettes: paletteRegistry, + setPalette: jest.fn(), + dataBounds: { min: 0, max: 100 }, + }; + }); + it('should be visible for predefined palettes', () => { + const instance = mountWithIntl(); + expect( + instance.find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_stops"]').exists() + ).toEqual(true); + }); + + it('should be visible for custom palettes', () => { + const instance = mountWithIntl( + - ); - expect( - instance.find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_stops"]').exists() - ).toEqual(true); - }); + }, + }} + /> + ); + expect( + instance.find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_stops"]').exists() + ).toEqual(true); }); }); }); diff --git a/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx b/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx index bc6a590db0cb7..1d1e212b87c0c 100644 --- a/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx +++ b/x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx @@ -108,16 +108,21 @@ export function CustomizablePalette({ colorStops: undefined, }; + const newColorStops = getColorStops(palettes, [], activePalette, dataBounds); if (isNewPaletteCustom) { - newParams.colorStops = getColorStops(palettes, [], activePalette, dataBounds); + newParams.colorStops = newColorStops; } newParams.stops = getPaletteStops(palettes, newParams, { prevPalette: isNewPaletteCustom || isCurrentPaletteCustom ? undefined : newPalette.name, dataBounds, + mapFromMinValue: true, }); + newParams.rangeMin = newColorStops[0].stop; + newParams.rangeMax = newColorStops[newColorStops.length - 1].stop; + setPalette({ ...newPalette, params: newParams, @@ -266,18 +271,18 @@ export function CustomizablePalette({ ) as RequiredPaletteParamTypes['rangeType']; const params: CustomPaletteParams = { rangeType: newRangeType }; + const { min: newMin, max: newMax } = getDataMinMax(newRangeType, dataBounds); + const { min: oldMin, max: oldMax } = getDataMinMax( + activePalette.params?.rangeType, + dataBounds + ); + const newColorStops = remapStopsByNewInterval(colorStopsToShow, { + oldInterval: oldMax - oldMin, + newInterval: newMax - newMin, + newMin, + oldMin, + }); if (isCurrentPaletteCustom) { - const { min: newMin, max: newMax } = getDataMinMax(newRangeType, dataBounds); - const { min: oldMin, max: oldMax } = getDataMinMax( - activePalette.params?.rangeType, - dataBounds - ); - const newColorStops = remapStopsByNewInterval(colorStopsToShow, { - oldInterval: oldMax - oldMin, - newInterval: newMax - newMin, - newMin, - oldMin, - }); const stops = getPaletteStops( palettes, { ...activePalette.params, colorStops: newColorStops, ...params }, @@ -285,8 +290,6 @@ export function CustomizablePalette({ ); params.colorStops = newColorStops; params.stops = stops; - params.rangeMin = newColorStops[0].stop; - params.rangeMax = newColorStops[newColorStops.length - 1].stop; } else { params.stops = getPaletteStops( palettes, @@ -294,6 +297,11 @@ export function CustomizablePalette({ { prevPalette: activePalette.name, dataBounds } ); } + // why not use newMin/newMax here? + // That's because there's the concept of continuity to accomodate, where in some scenarios it has to + // take into account the stop value rather than the data value + params.rangeMin = newColorStops[0].stop; + params.rangeMax = newColorStops[newColorStops.length - 1].stop; setPalette(mergePaletteParams(activePalette, params)); }} /> diff --git a/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts b/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts index 97dc2e45c96dc..07d93ca5c40c6 100644 --- a/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts +++ b/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts @@ -8,6 +8,7 @@ import { chartPluginMock } from 'src/plugins/charts/public/mocks'; import { applyPaletteParams, + getColorStops, getContrastColor, getDataMinMax, getPaletteStops, @@ -59,6 +60,78 @@ describe('applyPaletteParams', () => { }); }); +describe('getColorStops', () => { + const paletteRegistry = chartPluginMock.createPaletteRegistry(); + it('should return the same colorStops if a custom palette is passed, avoiding recomputation', () => { + const colorStops = [ + { stop: 0, color: 'red' }, + { stop: 100, color: 'blue' }, + ]; + expect( + getColorStops( + paletteRegistry, + colorStops, + { name: 'custom', type: 'palette' }, + { min: 0, max: 100 } + ) + ).toBe(colorStops); + }); + + it('should get a fresh list of colors', () => { + expect( + getColorStops( + paletteRegistry, + [ + { stop: 0, color: 'red' }, + { stop: 100, color: 'blue' }, + ], + { name: 'mocked', type: 'palette' }, + { min: 0, max: 100 } + ) + ).toEqual([ + { color: 'blue', stop: 0 }, + { color: 'yellow', stop: 50 }, + ]); + }); + + it('should get a fresh list of colors even if custom palette but empty colorStops', () => { + expect( + getColorStops(paletteRegistry, [], { name: 'mocked', type: 'palette' }, { min: 0, max: 100 }) + ).toEqual([ + { color: 'blue', stop: 0 }, + { color: 'yellow', stop: 50 }, + ]); + }); + + it('should correctly map the new colorStop to the current data bound and minValue', () => { + expect( + getColorStops( + paletteRegistry, + [], + { name: 'mocked', type: 'palette', params: { rangeType: 'number' } }, + { min: 100, max: 1000 } + ) + ).toEqual([ + { color: 'blue', stop: 100 }, + { color: 'yellow', stop: 550 }, + ]); + }); + + it('should reverse the colors', () => { + expect( + getColorStops( + paletteRegistry, + [], + { name: 'mocked', type: 'palette', params: { reverse: true } }, + { min: 100, max: 1000 } + ) + ).toEqual([ + { color: 'yellow', stop: 0 }, + { color: 'blue', stop: 50 }, + ]); + }); +}); + describe('remapStopsByNewInterval', () => { it('should correctly remap the current palette from 0..1 to 0...100', () => { expect( diff --git a/x-pack/plugins/lens/public/shared_components/coloring/utils.ts b/x-pack/plugins/lens/public/shared_components/coloring/utils.ts index b2969565f5390..413e3708e9c9b 100644 --- a/x-pack/plugins/lens/public/shared_components/coloring/utils.ts +++ b/x-pack/plugins/lens/public/shared_components/coloring/utils.ts @@ -269,11 +269,10 @@ export function getColorStops( palettes: PaletteRegistry, colorStops: Required['stops'], activePalette: PaletteOutput, - dataBounds: { min: number; max: number }, - defaultPalette?: string + dataBounds: { min: number; max: number } ) { // just forward the current stops if custom - if (activePalette?.name === CUSTOM_PALETTE) { + if (activePalette?.name === CUSTOM_PALETTE && colorStops?.length) { return colorStops; } // for predefined palettes create some stops, then drop the last one. diff --git a/x-pack/test/functional/apps/lens/table.ts b/x-pack/test/functional/apps/lens/table.ts index da079c0976db3..892534eec7033 100644 --- a/x-pack/test/functional/apps/lens/table.ts +++ b/x-pack/test/functional/apps/lens/table.ts @@ -122,7 +122,28 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(styleObj['background-color']).to.be('rgb(235, 239, 245)'); }); + it('should keep the coloring consistent when changing mode', async () => { + // Change mode from percent to number + await testSubjects.click('lnsPalettePanel_dynamicColoring_rangeType_groups_number'); + await PageObjects.header.waitUntilLoadingHasFinished(); + // check that all remained the same + const styleObj = await PageObjects.lens.getDatatableCellStyle(0, 2); + expect(styleObj['background-color']).to.be('rgb(235, 239, 245)'); + }); + + it('should keep the coloring consistent when moving to custom palette from default', async () => { + await PageObjects.lens.changePaletteTo('custom'); + await PageObjects.header.waitUntilLoadingHasFinished(); + // check that all remained the same + const styleObj = await PageObjects.lens.getDatatableCellStyle(0, 2); + expect(styleObj['background-color']).to.be('rgb(235, 239, 245)'); + }); + it('tweak the color stops numeric value', async () => { + // restore default palette and percent mode + await PageObjects.lens.changePaletteTo('temperature'); + await testSubjects.click('lnsPalettePanel_dynamicColoring_rangeType_groups_percent'); + // now tweak the value await testSubjects.setValue('lnsPalettePanel_dynamicColoring_stop_value_0', '30', { clearWithKeyboard: true, });