EnxDev commented on code in PR #41859:
URL: https://github.com/apache/superset/pull/41859#discussion_r3966583314
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -474,12 +474,7 @@ export default function transformProps(
const isMultiSeries = groupBy.length || metrics?.length > 1;
const xAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig];
- const xAxisType = getAxisType(
- stack,
- xAxisForceCategorical,
- xAxisDataType,
- seriesType,
- );
+ const xAxisType = getAxisType(stack, xAxisForceCategorical, xAxisDataType);
Review Comment:
**[P1] Preserve the old default for saved regular bar charts.** Explore
serializes every control value, including defaults, and this control already
defaulted to `false`. That means existing numeric bar charts will usually
arrive here with `xAxisForceCategorical: false`, not with the property omitted.
Before this change, `seriesType === Bar` still selected a Category axis; after
this call it selects Value and can bring the overflow back for charts whose
users never opted out. Regular charts also do not have the omitted-property
fallback added for Mixed charts. Could we add a migration/version sentinel so
only a `false` saved after this change means an explicit opt-out, and cover
both a legacy saved `false` and an omitted external payload?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts:
##########
@@ -330,3 +330,8 @@ test('x_axis_time_format should be hidden for numeric
columns', () => {
false,
);
});
+
+test('xAxisForceCategorical defaults to true for bar charts', () => {
+ const control = getControl('xAxisForceCategorical');
+ expect(control?.config?.default).toBe(true);
Review Comment:
The failing `lint-frontend` check comes from this line: `getControl` returns
`OverrideSharedControlItem | CustomControlItem`, and only the latter exposes
`config`. Could we narrow it with `isCustomControlItem` before reading the
default, as the new Mixed control-panel tests already do?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:
##########
@@ -63,6 +67,29 @@ import { StackControlsValue } from '../../../constants';
const { logAxis, minorSplitLine, truncateYAxis, yAxisBounds, orientation } =
DEFAULT_FORM_DATA;
+const barXAxisForceCategoricalControl = {
+ ...xAxisForceCategoricalControl,
+ config: {
+ ...xAxisForceCategoricalControl.config,
+ default: true,
Review Comment:
**[P1] Keep temporal bar charts on a Time axis by default.** This default is
unconditional, while the inherited control visibility includes temporal
columns. Previously `getAxisType` handled `GenericDataType.Temporal` before the
Bar coercion, so a normal temporal bar chart used `AxisType.Time`; with this
value it becomes Category and loses proportional spacing across missing dates.
The Mixed-chart default below deliberately limits the automatic category choice
to numeric axes. Could the regular Bar control use the same numeric-aware
default and add a temporal regression test?
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -65,6 +76,60 @@ const {
yAxisIndex,
} = DEFAULT_FORM_DATA;
+function hasBarSeriesInState(state: ControlPanelState | null): boolean {
+ const formData = state?.form_data;
+ const seriesA =
+ state?.controls?.seriesType?.value ?? formData?.seriesType ?? seriesType;
+ const seriesB =
+ state?.controls?.seriesTypeB?.value ?? formData?.seriesTypeB ?? seriesType;
+ return (
+ seriesA === EchartsTimeseriesSeriesType.Bar ||
+ seriesB === EchartsTimeseriesSeriesType.Bar
+ );
+}
+
+function isNumericXAxisState(state: ControlPanelState | null): boolean {
+ return checkColumnType(
+ getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn),
+ state?.controls?.datasource?.datasource,
+ [GenericDataType.Numeric],
+ );
+}
+
+const mixedXAxisForceCategoricalControl = {
+ ...xAxisForceCategoricalControl,
+ config: {
+ ...xAxisForceCategoricalControl.config,
+ description: t(
+ 'Treat values as categorical. Enabled by default when any query uses bar
series to prevent bar overflow and intermediate tick labels. Disable to use a
continuous numeric x-axis that preserves spacing for missing values.',
+ ),
+ initialValue: (control: ControlState, state: ControlPanelState | null) => {
Review Comment:
**[P1] Make the mixed-chart default follow a series change.** `initialValue`
is evaluated once and then removed by `getControlState`; it is not recalculated
when `seriesType` or `seriesTypeB` changes. A new Mixed chart starts as
Line/Line with this control set to `false`, so changing either query to Bar
leaves it false and the numeric bar uses a Value axis. That is the main
create-chart path described in the testing instructions. Could we wire the
dependency through recalculated control state while preserving a value the user
explicitly changed, and cover the Line-to-Bar interaction?
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -65,6 +76,60 @@ const {
yAxisIndex,
} = DEFAULT_FORM_DATA;
+function hasBarSeriesInState(state: ControlPanelState | null): boolean {
+ const formData = state?.form_data;
+ const seriesA =
+ state?.controls?.seriesType?.value ?? formData?.seriesType ?? seriesType;
+ const seriesB =
+ state?.controls?.seriesTypeB?.value ?? formData?.seriesTypeB ?? seriesType;
+ return (
+ seriesA === EchartsTimeseriesSeriesType.Bar ||
+ seriesB === EchartsTimeseriesSeriesType.Bar
+ );
+}
+
+function isNumericXAxisState(state: ControlPanelState | null): boolean {
+ return checkColumnType(
+ getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn),
+ state?.controls?.datasource?.datasource,
+ [GenericDataType.Numeric],
+ );
+}
+
+const mixedXAxisForceCategoricalControl = {
+ ...xAxisForceCategoricalControl,
+ config: {
+ ...xAxisForceCategoricalControl.config,
+ description: t(
+ 'Treat values as categorical. Enabled by default when any query uses bar
series to prevent bar overflow and intermediate tick labels. Disable to use a
continuous numeric x-axis that preserves spacing for missing values.',
+ ),
+ initialValue: (control: ControlState, state: ControlPanelState | null) => {
+ if (!isNumericXAxisState(state)) {
+ return control.value;
+ }
+ if (state?.form_data?.x_axis_sort !== undefined) {
+ return true;
+ }
+ if (control?.value !== undefined) {
+ return control.value;
+ }
+ return hasBarSeriesInState(state);
+ },
+ },
+};
+
+const mixedSharedQuerySection: ControlPanelSectionConfig = {
+ label: t('Shared query fields'),
+ expanded: true,
+ controlSetRows: [
+ ['x_axis'],
+ ['time_grain_sqla'],
+ [mixedXAxisForceCategoricalControl],
+ [xAxisSortControl],
Review Comment:
**[P1] Wire these sort controls before exposing them.** MixedTimeseries
`buildQuery.ts` never invokes `sortOperator` (the regular Timeseries builder
does), and its `transformProps.ts` neither reads `xAxisSort`/`xAxisSortAsc` nor
passes `xAxisSortSeries` to `extractSeries`. As a result, both new controls
update form data but cannot change query order or rendered category order.
Could we add the single-series query sort and multi-series client sort paths,
with regressions for both queries?
--
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]
