Skip to content

Commit

Permalink
[Lens] Fix transition to custom palette inconsistency when in number …
Browse files Browse the repository at this point in the history 
…mode (#110852)

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
@dej611 @kibanamachine
dej611 and kibanamachine authored Sep 3, 2021
1 parent 9ba00ee commit 21b4752
Show file tree
Hide file tree
Showing 6 changed files with 246 additions and 76 deletions.
5 changes: 4 additions & 1 deletion src/plugins/charts/public/services/palettes/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
182 changes: 124 additions & 58 deletions x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
*/

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';
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', () => {
Expand Down Expand Up @@ -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(<CustomizablePalette {...props} />);

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(<CustomizablePalette {...props} />);

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(<CustomizablePalette {...props} />);

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(<CustomizablePalette {...props} />);
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(<CustomizablePalette {...props} />);
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(
<CustomizablePalette
{...props}
activePalette={{
type: 'palette',
expect(props.setPalette).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
params: expect.objectContaining({
rangeType: 'number',
rangeMin: 5,
rangeMax: 102.5 /* (200 - (200-5)/ colors.length: 2) */,
}),
})
);

expect(props.setPalette).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
params: expect.objectContaining({
rangeType: 'percent',
rangeMin: 0,
rangeMax: 50 /* 100 - (100-0)/ colors.length: 2 */,
}),
})
);
});
});

describe('custom stops', () => {
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(<CustomizablePalette {...props} />);
expect(
instance.find('[data-test-subj="lnsPalettePanel_dynamicColoring_custom_stops"]').exists()
).toEqual(true);
});

it('should be visible for custom palettes', () => {
const instance = mountWithIntl(
<CustomizablePalette
{...props}
activePalette={{
type: 'palette',
name: 'custom',
params: {
name: 'custom',
params: {
name: 'custom',
},
}}
/>
);
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);
});
});
});
36 changes: 22 additions & 14 deletions x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -266,34 +271,37 @@ 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 },
{ dataBounds }
);
params.colorStops = newColorStops;
params.stops = stops;
params.rangeMin = newColorStops[0].stop;
params.rangeMax = newColorStops[newColorStops.length - 1].stop;
} else {
params.stops = getPaletteStops(
palettes,
{ ...activePalette.params, ...params },
{ 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));
}}
/>
Expand Down
73 changes: 73 additions & 0 deletions x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import { chartPluginMock } from 'src/plugins/charts/public/mocks';
import {
applyPaletteParams,
getColorStops,
getContrastColor,
getDataMinMax,
getPaletteStops,
Expand Down Expand Up @@ -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(
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugins/lens/public/shared_components/coloring/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,10 @@ export function getColorStops(
palettes: PaletteRegistry,
colorStops: Required<CustomPaletteParams>['stops'],
activePalette: PaletteOutput<CustomPaletteParams>,
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.
Expand Down
Loading

0 comments on commit 21b4752

Please sign in to comment.