Skip to content

Commit

Permalink
[Lens] Save valuesInLegend:true as legendStats:['value'] in prepa…
Browse files Browse the repository at this point in the history
…ration for legend statistics (#181953)

## Summary

To the code owners,

This PR updates the saved object of Lens with the following changes: the
property `valuesInLegend: boolean` is converted to the new property
`legendStats: ["currentAndLastValue"]` (cartesian) or `legendStats:
["value"]`(partition) to accommodate displaying additional legend
statistics in the future. There are no user-facing changes introduced. I
have updated all saved object shapes in the code to adhere to the new
structure. While the old structure will continue to function properly,
this update ensures cleaner code.

Fixes #181035 (phase 2). It's is
a continuation of #180917.

In comparison to the phase 1, we've removed the conversion of
legendStats to valuesInLegend which was initially added in the previous
PR to ensure backward compatibility for serverless applications.

<img width="807" alt="Screenshot 2024-04-25 at 10 51 50"
src="https://github.com/elastic/kibana/assets/4283304/6c23d035-f64d-4fb1-96c6-d30392ae6752">

I won't merge till the end of the week as it has to be released
separately from phase 1.

---------

Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com>
Co-authored-by: Alex Szabo <alex.szabo@elastic.co>
  • Loading branch information
3 people committed May 3, 2024
1 parent 66ab2c3 commit ab065ad
Show file tree
Hide file tree
Showing 26 changed files with 144 additions and 109 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ import type {
DatatableColumnMeta,
ExpressionFunctionDefinition,
} from '@kbn/expressions-plugin/common';
import { LegendSize, XYLegendValue } from '@kbn/visualizations-plugin/common';
import {
LegendSize,
ExpressionValueVisDimension,
XYLegendValue,
} from '@kbn/visualizations-plugin/common';
import { EventAnnotationOutput } from '@kbn/event-annotation-plugin/common';
import { ExpressionValueVisDimension } from '@kbn/visualizations-plugin/common';

import { MakeOverridesSerializable, Simplify } from '@kbn/chart-expressions-common/types';
import {
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { cloneDeep } from 'lodash';
import { PartitionLegendValue } from '@kbn/visualizations-plugin/common/constants';
import { PieLayerState, PieVisualizationState } from '../../../common/types';

type PersistedPieLayerState = Omit<PieLayerState, 'legendStats'> & {
type PersistedPieLayerState = PieLayerState & {
showValuesInLegend?: boolean;
};

Expand Down Expand Up @@ -38,16 +38,3 @@ function convertToLegendStats(state: PieVisualizationState) {

return state;
}

export function convertToPersistable(state: PieVisualizationState) {
const newState = cloneDeep(state) as unknown as PersistedPieVisualizationState;

newState.layers.forEach((l) => {
if ('legendStats' in l && Array.isArray(l.legendStats)) {
l.showValuesInLegend = l.legendStats.includes(PartitionLegendValue.Value);
delete l.legendStats;
}
});

return newState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -205,35 +205,6 @@ describe('pie_visualization', () => {
});
});

describe('#getPersistableState', () => {
describe('converting to legend stats', () => {
it('converts `legendStats` to `valuesInLegend`', () => {
const runtimeState = getExampleState();
// no legend stats at all
const { state } = pieVisualization.getPersistableState!(runtimeState);
expect('legendStats' in state.layers[0]).toBeFalsy();
expect(state.layers[0].showValuesInLegend).toEqual(undefined);

// legend stats === ['value']
runtimeState.layers[0].legendStats = [PartitionLegendValue.Value];
const { state: stateWithShowValuesInLegendTrue } =
pieVisualization.getPersistableState!(runtimeState);

expect('legendStats' in stateWithShowValuesInLegendTrue.layers[0]).toBeFalsy();
expect(stateWithShowValuesInLegendTrue.layers[0].showValuesInLegend).toEqual(true);

// legend stats === ['value']
runtimeState.layers[0].legendStats = [];

const { state: stateWithShowValuesInLegendFalse } =
pieVisualization.getPersistableState!(runtimeState);

expect('legendStats' in stateWithShowValuesInLegendFalse.layers[0]).toBeFalsy();
expect(stateWithShowValuesInLegendFalse.layers[0].showValuesInLegend).toEqual(false);
});
});
});

describe('#removeDimension', () => {
it('removes corresponding collapse function if exists', () => {
const state = getExampleState();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,7 @@ import { checkTableForContainsSmallValues } from './render_helpers';
import { DatasourcePublicAPI } from '../..';
import { nonNullable, getColorMappingDefaults } from '../../utils';
import { getColorMappingTelemetryEvents } from '../../lens_ui_telemetry/color_telemetry_helpers';
import {
PersistedPieVisualizationState,
convertToPersistable,
convertToRuntime,
} from './persistence';
import { PersistedPieVisualizationState, convertToRuntime } from './persistence';

const metricLabel = i18n.translate('xpack.lens.pie.groupMetricLabelSingular', {
defaultMessage: 'Metric',
Expand Down Expand Up @@ -183,10 +179,6 @@ export const getPieVisualization = ({
};
},

getPersistableState(state) {
return { savedObjectReferences: [], state: convertToPersistable(state) };
},

getMainPalette: (state) => {
if (!state) {
return undefined;
Expand Down
16 changes: 1 addition & 15 deletions x-pack/plugins/lens/public/visualizations/xy/persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import { LegendConfig } from '@kbn/visualizations-plugin/common';
import { v4 as uuidv4 } from 'uuid';
import type { SavedObjectReference } from '@kbn/core/public';
import { EVENT_ANNOTATION_GROUP_TYPE } from '@kbn/event-annotation-common';
Expand Down Expand Up @@ -83,7 +82,6 @@ export type XYPersistedLayerConfig =
export type XYPersistedState = Omit<XYState, 'layers'> & {
layers: XYPersistedLayerConfig[];
valuesInLegend?: boolean;
legend: Omit<LegendConfig, 'legendStats'>;
};

export function convertToRuntime(
Expand All @@ -97,7 +95,7 @@ export function convertToRuntime(
}

export function convertToPersistable(state: XYState) {
const persistableState: XYPersistedState = convertToValuesInLegend(state);
const persistableState: XYPersistedState = state;
const savedObjectReferences: SavedObjectReference[] = [];
const persistableLayers: XYPersistedLayerConfig[] = [];

Expand Down Expand Up @@ -291,15 +289,3 @@ function convertToLegendStats(state: XYState & { valuesInLegend?: unknown }) {
}
return state;
}

function convertToValuesInLegend(state: XYState) {
const newState: XYPersistedState = cloneDeep(state);

if ('legendStats' in newState.legend && Array.isArray(newState.legend.legendStats)) {
newState.valuesInLegend = newState.legend.legendStats.includes(
XYLegendValue.CurrentAndLastValue
);
delete newState.legend.legendStats;
}
return newState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3837,23 +3837,6 @@ describe('xy_visualization', () => {
},
]);
});

it('should transform legendStats to valuesInLegend', () => {
const state = exampleState();
const { state: noLegendStatsState } = xyVisualization.getPersistableState!(state);
expect(noLegendStatsState.legend.legendStats).not.toBeDefined();
expect(noLegendStatsState.valuesInLegend).not.toBeDefined();

state.legend.legendStats = [XYLegendValue.CurrentAndLastValue];
const { state: legendStatsState } = xyVisualization.getPersistableState!(state);
expect(legendStatsState.legend.legendStats).not.toBeDefined();
expect(legendStatsState.valuesInLegend).toEqual(true);

state.legend.legendStats = [];
const { state: legendStatsStateFalsy } = xyVisualization.getPersistableState!(state);
expect(legendStatsStateFalsy.legend.legendStats).not.toBeDefined();
expect(legendStatsStateFalsy.valuesInLegend).toEqual(false);
});
});

describe('getSupportedActionsForLayer', () => {
Expand Down

Large diffs are not rendered by default.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const visConfig = {
categoryDisplay: 'default',
legendDisplay: 'hide',
numberDisplay: 'percent',
showValuesInLegend: true,
legendStats: ['value'],
nestedLegend: false,
layerType: 'data',
},
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const getAlertsHistogramLensAttributes: GetLensAttributes = (
isVisible: true,
position: 'right',
legendSize: 'xlarge',
legendStats: ['currentAndLastValue'],
},
valueLabels: 'hide',
preferredSeriesType: 'bar_stacked',
Expand All @@ -49,7 +50,6 @@ export const getAlertsHistogramLensAttributes: GetLensAttributes = (
yLeft: false,
yRight: true,
},
valuesInLegend: true,
},
query: {
query: '',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const getRulePreviewLensAttributes: GetLensAttributes = (
legend: {
isVisible: extraOptions?.showLegend,
position: 'right',
legendStats: ['currentAndLastValue'],
},
valueLabels: 'hide',
preferredSeriesType: 'bar_stacked',
Expand All @@ -39,7 +40,6 @@ export const getRulePreviewLensAttributes: GetLensAttributes = (
splitAccessor: 'e92c8920-0449-4564-81f4-8945517817a4',
},
],
valuesInLegend: true,
yTitle: '',
axisTitlesVisibilitySettings: {
x: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,9 @@ describe('getEventsHistogramLensAttributes', () => {
);

expect(result?.current?.state?.visualization).toEqual(
expect.objectContaining({ valuesInLegend: true })
expect.objectContaining({
legend: expect.objectContaining({ legendStats: ['currentAndLastValue'] }),
})
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const getEventsHistogramLensAttributes: GetLensAttributes = (
isVisible: true,
position: 'right',
legendSize: 'xlarge',
legendStats: ['currentAndLastValue'],
},
valueLabels: 'hide',
preferredSeriesType: 'bar_stacked',
Expand All @@ -50,7 +51,6 @@ export const getEventsHistogramLensAttributes: GetLensAttributes = (
yLeft: false,
yRight: true,
},
valuesInLegend: true,
},
query: {
query: '',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ describe('getExternalAlertLensAttributes', () => {
);

expect(result?.current?.state?.visualization).toEqual(
expect.objectContaining({ valuesInLegend: true })
expect.objectContaining({
legend: expect.objectContaining({ legendStats: ['currentAndLastValue'] }),
})
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const getExternalAlertLensAttributes: GetLensAttributes = (
isVisible: true,
position: 'right',
legendSize: 'xlarge',
legendStats: ['currentAndLastValue'],
},
valueLabels: 'hide',
preferredSeriesType: 'bar_stacked',
Expand All @@ -48,7 +49,6 @@ export const getExternalAlertLensAttributes: GetLensAttributes = (
yLeft: false,
yRight: true,
},
valuesInLegend: true,
},
query: {
query: '',
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ describe('getDnsTopDomainsLensAttributes', () => {

it('should render values in legend', () => {
expect(result?.current?.state?.visualization).toEqual(
expect.objectContaining({ valuesInLegend: true })
expect.objectContaining({
legend: expect.objectContaining({ legendStats: ['currentAndLastValue'] }),
})
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const getDnsTopDomainsLensAttributes: GetLensAttributes = (
isVisible: true,
position: 'right',
legendSize: 'xlarge',
legendStats: ['currentAndLastValue'],
},
valueLabels: 'hide',
fittingFunction: 'None',
Expand All @@ -36,7 +37,6 @@ export const getDnsTopDomainsLensAttributes: GetLensAttributes = (
yLeft: false,
yRight: false,
},
valuesInLegend: true,
tickLabelsVisibilitySettings: {
x: true,
yLeft: true,
Expand Down

0 comments on commit ab065ad

Please sign in to comment.