Skip to content

Commit 77de0a1

Browse files
Greg Neighborsclaude
authored andcommitted
refactor(waterfall): use single echarts axis show flag per review
Address review feedback (@SBIN2010): echarts `xAxis.show` / `yAxis.show` hides the entire axis component — line, ticks, labels, name and gridlines — in one flag (CartesianAxisView returns early when `show` is false), so the previous per-sub-flag approach was redundant. Also gate the axis-specific settings (labels, formats, tick layout) with `visibility` so they no longer clutter the panel once an axis is hidden. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 94755dd commit 77de0a1

3 files changed

Lines changed: 27 additions & 24 deletions

File tree

superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,8 @@ const config: ControlPanelConfig = {
195195
label: t('X Axis Label'),
196196
renderTrigger: true,
197197
default: '',
198+
visibility: ({ controls }) =>
199+
controls?.show_x_axis?.value !== false,
198200
},
199201
},
200202
],
@@ -205,6 +207,8 @@ const config: ControlPanelConfig = {
205207
...sharedControls.x_axis_time_format,
206208
default: DEFAULT_TIME_FORMAT,
207209
description: `${D3_TIME_FORMAT_DOCS}.`,
210+
visibility: ({ controls }) =>
211+
controls?.show_x_axis?.value !== false,
208212
},
209213
},
210214
],
@@ -225,6 +229,8 @@ const config: ControlPanelConfig = {
225229
clearable: false,
226230
renderTrigger: true,
227231
description: t('The way the ticks are laid out on the X-axis'),
232+
visibility: ({ controls }) =>
233+
controls?.show_x_axis?.value !== false,
228234
},
229235
},
230236
],
@@ -256,6 +262,8 @@ const config: ControlPanelConfig = {
256262
label: t('Y Axis Label'),
257263
renderTrigger: true,
258264
default: '',
265+
visibility: ({ controls }) =>
266+
controls?.show_y_axis?.value !== false,
259267
},
260268
},
261269
],
@@ -265,6 +273,12 @@ const config: ControlPanelConfig = {
265273
},
266274
],
267275
controlOverrides: {
276+
y_axis_format: {
277+
visibility: ({ controls }) => controls?.show_y_axis?.value !== false,
278+
},
279+
currency_format: {
280+
visibility: ({ controls }) => controls?.show_y_axis?.value !== false,
281+
},
268282
groupby: {
269283
label: t('Breakdowns'),
270284
description:

superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,6 @@ export default function transformProps(
372372
}
373373
axisLabel.formatter = xAxisFormatter;
374374
axisLabel.hideOverlap = false;
375-
axisLabel.show = showXAxis;
376375

377376
const seriesProps: Pick<BarSeriesOption, 'type' | 'stack' | 'emphasis'> = {
378377
type: 'bar',
@@ -446,29 +445,26 @@ export default function transformProps(
446445
data: [legendNames.INCREASE, legendNames.DECREASE, legendNames.TOTAL],
447446
},
448447
xAxis: {
448+
show: showXAxis,
449449
data: xAxisData,
450450
type: 'category',
451-
name: showXAxis ? xAxisLabel : '',
451+
name: xAxisLabel,
452452
nameTextStyle: {
453453
padding: [theme.sizeUnit * 4, 0, 0, 0],
454454
},
455455
nameLocation: 'middle',
456456
axisLabel,
457-
axisLine: { show: showXAxis },
458-
axisTick: { show: showXAxis },
459457
},
460458
yAxis: {
461459
...defaultYAxis,
460+
show: showYAxis,
462461
type: 'value',
463462
nameTextStyle: {
464463
padding: [0, 0, theme.sizeUnit * 5, 0],
465464
},
466465
nameLocation: 'middle',
467-
name: showYAxis ? yAxisLabel : '',
468-
axisLabel: { show: showYAxis, formatter: defaultFormatter },
469-
axisLine: { show: showYAxis },
470-
axisTick: { show: showYAxis },
471-
splitLine: { show: showYAxis },
466+
name: yAxisLabel,
467+
axisLabel: { formatter: defaultFormatter },
472468
},
473469
tooltip: {
474470
...getDefaultTooltip(refs),

superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/transformProps.test.ts

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -186,25 +186,18 @@ const buildAxes = (extraFormData: Record<string, unknown>) => {
186186

187187
test('shows both axes by default', () => {
188188
const { xAxis, yAxis } = buildAxes({});
189-
expect(xAxis.axisLabel.show).not.toBe(false);
190-
expect(xAxis.axisLine.show).not.toBe(false);
191-
expect(yAxis.axisLabel.show).not.toBe(false);
192-
expect(yAxis.splitLine.show).not.toBe(false);
189+
expect(xAxis.show).not.toBe(false);
190+
expect(yAxis.show).not.toBe(false);
193191
});
194192

195-
test('hides the X axis chrome when showXAxis is false', () => {
193+
test('hides the whole X axis when showXAxis is false', () => {
196194
const { xAxis } = buildAxes({ showXAxis: false });
197-
expect(xAxis.axisLabel.show).toBe(false);
198-
expect(xAxis.axisLine.show).toBe(false);
199-
expect(xAxis.axisTick.show).toBe(false);
200-
expect(xAxis.name).toBe('');
195+
// echarts hides the axis line, ticks, labels, name, and gridlines when
196+
// `show` is false — a single flag rather than a set of sub-flags.
197+
expect(xAxis.show).toBe(false);
201198
});
202199

203-
test('hides the Y axis chrome when showYAxis is false', () => {
200+
test('hides the whole Y axis when showYAxis is false', () => {
204201
const { yAxis } = buildAxes({ showYAxis: false });
205-
expect(yAxis.axisLabel.show).toBe(false);
206-
expect(yAxis.axisLine.show).toBe(false);
207-
expect(yAxis.axisTick.show).toBe(false);
208-
expect(yAxis.splitLine.show).toBe(false);
209-
expect(yAxis.name).toBe('');
202+
expect(yAxis.show).toBe(false);
210203
});

0 commit comments

Comments
 (0)