rusackas commented on code in PR #42599:
URL: https://github.com/apache/superset/pull/42599#discussion_r3685556416
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -994,4 +994,56 @@ describe('Bar Chart X-axis Time Formatting', () => {
expect(grid.bottom).not.toBe(expandedPadding.bottom);
});
});
+
+ describe('Regression test for Issue #42560', () => {
+ test('custom X Axis Title is preserved verbatim, not overwritten by the
axis number/currency format ("unit")', () => {
+ const formData = {
+ ...baseFormData,
+ orientation: 'vertical',
+ xAxisTitle: 'My X Axis',
+ xAxisNumberFormat: 'SMART_NUMBER',
+ yAxisFormat: '$,.2f',
+ };
+
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData,
Review Comment:
Good catch, fixed! The fixture was using the inherited temporal data so
`xAxisNumberFormat` never actually hit `getNumberFormatter`. Swapped in a
numeric x-axis column so it does.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -994,4 +994,56 @@ describe('Bar Chart X-axis Time Formatting', () => {
expect(grid.bottom).not.toBe(expandedPadding.bottom);
});
});
+
+ describe('Regression test for Issue #42560', () => {
+ test('custom X Axis Title is preserved verbatim, not overwritten by the
axis number/currency format ("unit")', () => {
+ const formData = {
+ ...baseFormData,
+ orientation: 'vertical',
+ xAxisTitle: 'My X Axis',
+ xAxisNumberFormat: 'SMART_NUMBER',
+ yAxisFormat: '$,.2f',
+ };
+
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+
+ expect(xAxis.name).toBe('My X Axis');
+ });
+
+ test('X Axis Title control maps onto the rendered category (left) axis in
horizontal orientation, not the bottom axis', () => {
+ // Documents the axis swap for horizontal bar charts: `xAxisTitle` ends
+ // up on `echartOptions.yAxis.name` (the vertical category axis) and
+ // `yAxisTitle` ends up on `echartOptions.xAxis.name` (the horizontal
+ // value axis). This is existing, intentional swap behavior, not the
+ // "unit" overwrite described in the issue.
+ const formData = {
+ ...baseFormData,
+ orientation: 'horizontal',
+ xAxisTitle: 'My X Axis',
+ yAxisTitle: 'My Y Axis',
+ };
+
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData,
Review Comment:
Fixed, swapped in categorical data and added a `type` assertion so this is
actually proving the category-axis mapping instead of a temporal axis wearing a
category label.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -994,4 +994,56 @@ describe('Bar Chart X-axis Time Formatting', () => {
expect(grid.bottom).not.toBe(expandedPadding.bottom);
});
});
+
+ describe('Regression test for Issue #42560', () => {
+ test('custom X Axis Title is preserved verbatim, not overwritten by the
axis number/currency format ("unit")', () => {
+ const formData = {
+ ...baseFormData,
+ orientation: 'vertical',
+ xAxisTitle: 'My X Axis',
+ xAxisNumberFormat: 'SMART_NUMBER',
+ yAxisFormat: '$,.2f',
+ };
+
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+
+ expect(xAxis.name).toBe('My X Axis');
Review Comment:
That's intentional, this is a `transformProps` unit test, not a full
render-through-ECharts one. As the PR description notes, CI green here doesn't
close #42560, it just rules out the number-format theory in this code path. A
full control-to-render repro would be a separate, heavier test, worth doing if
the bug's still showing up once this lands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
