codeant-ai-for-open-source[bot] commented on code in PR #42599:
URL: https://github.com/apache/superset/pull/42599#discussion_r3684761816
##########
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:
Agreed—the test should use categorical query data so it validates the
behavior it describes, rather than relying on inherited timestamp data.
Please override the fixture’s query data with a non-temporal dimension and
assert the rendered axis type as well as the title. For example:
```typescript
const chartProps = new ChartProps({
...baseChartPropsConfig,
formData,
queriesData: [
{
data: [
{ category: 'A', value: 10 },
{ category: 'B', value: 20 },
],
},
],
});
```
Then assert:
```typescript
expect(renderedYAxis.type).toBe('category');
expect(renderedYAxis.name).toBe('My X Axis');
expect(renderedXAxis.name).toBe('My Y Axis');
```
The existing test name and comment can remain once the fixture is
categorical: they accurately document that, for horizontal bars, the
user-facing `xAxisTitle` is rendered on the category (`yAxis`) axis. Without
that fixture change, renaming the test to describe only a type-independent axis
swap would be more accurate, but would not cover the intended category-axis
behavior.
##########
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:
Agreed. The current fixture inherits temporal `__timestamp` data, so
`xAxisNumberFormat: 'SMART_NUMBER'` is not exercised; the assertion only proves
that `xAxisTitle` is assigned directly.
The test should override the query fixture with a numeric X-axis column and
point the form data at it. For example:
```typescript
const formData = {
...baseFormData,
orientation: 'vertical',
groupby: ['x'],
xAxisTitle: 'My X Axis',
xAxisNumberFormat: 'SMART_NUMBER',
yAxisFormat: '$,.2f',
};
const chartProps = new ChartProps({
...baseChartPropsConfig,
formData,
queriesData: [
{
...baseChartPropsConfig.queriesData[0],
data: [
{ x: 1000, metric: 10 },
{ x: 2000, metric: 20 },
],
},
],
});
```
The exact numeric column name should match the fixture’s existing
X-axis/query-field configuration. The important part is that the X-axis value
is numeric and no longer `__timestamp`, so the numeric formatter path is
selected. The test can then continue asserting:
```typescript
expect((transformedProps.echartOptions.xAxis as any).name).toBe(
'My X Axis',
);
```
This change is necessary for the regression test to validate its stated
purpose; otherwise it does not cover numeric/unit formatting at all.
--
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]
