Skip to content

Commit 288c6a9

Browse files
authored
[BUGFIX]: resolves saving issue of #3784 (#525)
* [BUGFIX]: resolves saving issue of #3784 This should resolve the issue with the backend validation for the 'none' values, by not passing down the none at all to the json payload. It keeps using a 'none' label as a default field in the form for UX/clarity. It also removes some redundant objects and variables that are should not be necessary --------- Signed-off-by: Simon Olander <simon.olander@sap.com> * [IGNORE] adds test-case for validating log based echart yaxis conversion --------- Signed-off-by: Simon Olander <simon.olander@sap.com> --------- Signed-off-by: Simon Olander <simon.olander@sap.com>
1 parent 5662acd commit 288c6a9

5 files changed

Lines changed: 55 additions & 27 deletions

File tree

timeserieschart/src/TimeSeriesChartPanel.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ export function TimeSeriesChartPanel(props: TimeSeriesChartProps): ReactElement
123123
}, [props.spec.visual]);
124124

125125
// Use the logBase from yAxis options, defaulting to 'none' if not set
126-
const useLogarithmicBase: LOG_BASE = yAxis?.logBase ?? 'none';
126+
const useLogarithmicBase: LOG_BASE = yAxis?.logBase;
127127

128128
// convert Perses dashboard format to be ECharts compatible
129129
const echartsYAxis = useMemo(() => {

timeserieschart/src/YAxisOptionsEditor.tsx

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,8 @@ import {
2020
DEFAULT_Y_AXIS,
2121
TimeSeriesChartYAxisOptions,
2222
Y_AXIS_CONFIG,
23-
LOG_BASE_OPTIONS,
2423
LOG_BASE_CONFIG,
25-
LOG_VALID_BASES,
24+
LOG_BASE_OPTIONS,
2625
} from './time-series-chart-model';
2726

2827
export interface YAxisOptionsEditorProps {
@@ -31,7 +30,7 @@ export interface YAxisOptionsEditorProps {
3130
}
3231

3332
export function YAxisOptionsEditor({ value, onChange }: YAxisOptionsEditorProps): ReactElement {
34-
const logBase = LOG_BASE_CONFIG[LOG_VALID_BASES[value.logBase ?? 'none']];
33+
const logBase = value.logBase ? LOG_BASE_CONFIG[value.logBase] : undefined;
3534

3635
return (
3736
<OptionsEditorGroup title="Y Axis">
@@ -64,13 +63,13 @@ export function YAxisOptionsEditor({ value, onChange }: YAxisOptionsEditorProps)
6463
<SettingsAutocomplete
6564
value={{
6665
...logBase,
67-
id: logBase.label,
66+
id: logBase?.label ?? 'None',
6867
}}
6968
options={LOG_BASE_OPTIONS}
7069
onChange={(__, newValue) => {
7170
const updatedValue: TimeSeriesChartYAxisOptions = {
7271
...value,
73-
logBase: newValue.log,
72+
logBase: newValue.log ?? undefined,
7473
};
7574
onChange(updatedValue);
7675
}}

timeserieschart/src/time-series-chart-model.ts

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export const DEFAULT_Y_AXIS: TimeSeriesChartYAxisOptions = {
8888
format: DEFAULT_FORMAT,
8989
min: undefined,
9090
max: undefined,
91-
logBase: 'none',
91+
logBase: undefined,
9292
};
9393

9494
export const Y_AXIS_CONFIG = {
@@ -187,27 +187,21 @@ export const OPACITY_CONFIG = {
187187
};
188188

189189
// LogBase outlines the allowed log bases for the log-supported charts.
190-
export type LOG_BASE_LABEL = 'none' | 'log2' | 'log10';
191-
export type LOG_BASE = 'none' | 2 | 10;
190+
export type LOG_BASE = undefined | 2 | 10;
192191

193192
// Single source of truth for log base configuration
194-
export const LOG_BASE_CONFIG: Record<LOG_BASE_LABEL, { label: string; log: LOG_BASE }> = {
195-
none: { label: 'None', log: 'none' },
196-
log2: { label: '2', log: 2 },
197-
log10: { label: '10', log: 10 },
193+
export const LOG_BASE_CONFIG: Record<string, { label: string; log: LOG_BASE }> = {
194+
none: { label: 'None', log: undefined },
195+
'2': { label: '2', log: 2 },
196+
'10': { label: '10', log: 10 },
198197
};
199198

200199
// Options array for SettingsAutocomplete
201200
export const LOG_BASE_OPTIONS = Object.entries(LOG_BASE_CONFIG).map(([id, config]) => ({
202-
id: id as LOG_BASE_LABEL,
201+
id: id as string,
203202
...config,
204203
}));
205204

206-
// Reverse lookup map from LOG_BASE value to LOG_BASE_LABEL
207-
export const LOG_VALID_BASES: Record<LOG_BASE, LOG_BASE_LABEL> = Object.fromEntries(
208-
Object.entries(LOG_BASE_CONFIG).map(([label, config]) => [config.log, label])
209-
) as Record<LOG_BASE, LOG_BASE_LABEL>;
210-
211205
// Both of these constants help produce a value that is LESS THAN the initial value.
212206
// For positive values, we multiply by a number less than 1 to get this outcome.
213207
// For negative values, we multiply to a number greater than 1 to get this outcome.

timeserieschart/src/utils/data-transform.test.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ describe('convertPanelYAxis', () => {
5454
min: 0.1,
5555
max: 1,
5656
};
57-
const echartsAxis = convertPanelYAxis(persesAxis, 'none');
57+
const echartsAxis = convertPanelYAxis(persesAxis, undefined);
5858
// Axis label is handled outside of echarts since it is built with a custom React component.
5959
expect(echartsAxis).toEqual({
6060
show: true,
@@ -65,6 +65,42 @@ describe('convertPanelYAxis', () => {
6565
},
6666
});
6767
});
68+
it('should convert a Perses yAxis spec of type log to the ECharts equivalent', () => {
69+
const persesAxis: TimeSeriesChartYAxisOptions = {
70+
show: true,
71+
label: 'Axis Label',
72+
format: {
73+
unit: 'percent-decimal',
74+
decimalPlaces: 0,
75+
},
76+
min: 0.1,
77+
max: 1,
78+
};
79+
const actualAxisLog2 = convertPanelYAxis(persesAxis, 2);
80+
// Axis label is handled outside of echarts since it is built with a custom React component.
81+
expect(actualAxisLog2).toEqual({
82+
show: true,
83+
max: 1,
84+
min: undefined,
85+
type: 'log',
86+
logBase: 2,
87+
axisLabel: {
88+
show: true,
89+
},
90+
});
91+
const actualAxisLog10 = convertPanelYAxis(persesAxis, 10);
92+
// Axis label is handled outside of echarts since it is built with a custom React component.
93+
expect(actualAxisLog10).toEqual({
94+
show: true,
95+
max: 1,
96+
min: undefined,
97+
type: 'log',
98+
logBase: 10,
99+
axisLabel: {
100+
show: true,
101+
},
102+
});
103+
});
68104
});
69105

70106
const ROUND_DOWN_TESTS = [

timeserieschart/src/utils/data-transform.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -234,19 +234,18 @@ function findMax(data: LegacyTimeSeries[] | TimeSeries[]): number {
234234
*/
235235
export function convertPanelYAxis(
236236
inputAxis: TimeSeriesChartYAxisOptions = {},
237-
useLogarithmicBase: LOG_BASE
237+
useLogarithmicBase: LOG_BASE | undefined
238238
): YAXisComponentOption {
239239
// Determine the appropriate min value based on scale type and user input
240240
let minValue: YAXisComponentOption['min'];
241-
242-
if (inputAxis?.min !== undefined) {
243-
// User explicitly set a min value - use it for both linear and log scales
244-
minValue = inputAxis.min;
245-
} else if (useLogarithmicBase !== 'none') {
241+
if (useLogarithmicBase !== undefined) {
246242
// For logarithmic scales without explicit min:
247243
// Let ECharts auto-calculate the range based on data to avoid issues with
248244
// function-based calculations which can result in improper ranges (e.g., 1-10)
249245
minValue = undefined;
246+
} else if (inputAxis?.min !== undefined) {
247+
// User explicitly set a min value - use it for both linear and log scales
248+
minValue = inputAxis.min;
250249
} else {
251250
// For linear scales without explicit min:
252251
// Use dynamic calculation with padding for better visualization
@@ -278,7 +277,7 @@ export function convertPanelYAxis(
278277
};
279278

280279
// Apply logarithmic scale settings if requested
281-
if (useLogarithmicBase !== 'none') {
280+
if (useLogarithmicBase !== undefined) {
282281
return {
283282
...yAxis,
284283
type: 'log',

0 commit comments

Comments
 (0)