Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(plugin-chart-echarts): support truncated numeric x-axis #26215

Merged
merged 3 commits into from
Dec 8, 2023
Merged

fix(plugin-chart-echarts): support truncated numeric x-axis #26215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a85725
feat(plugin-chart-echarts): support truncated numeric x-axis
villebro Dec 8, 2023
72a73da
fix type
villebro Dec 8, 2023
cc82fcf
Ignore control for other axis types
michael-s-molina Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const DEFAULT_FORM_DATA: Partial<EchartsBubbleFormData> = {
logYAxis: false,
xAxisTitleMargin: 30,
yAxisTitleMargin: 30,
truncateXAxis: false,
Copy link
Member Author

@villebro villebro Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that for bubble chart, I'm assuming truncation is disabled by default, as it's more likely people want to see it starting from origo in this case (having a truncated x axis but a non-truncated y axis doesn't make sense for a scatter plot IMO).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't necessarily agree, I think it's lower-risk to have truncation as the default here too. If the user's data is suited for showing the origin, then I think it will look okay with the default truncation, but the converse is not true.

The x-axis in a scatterplot could be temperature, sales $, counts, or anything else where the difference between zero and the min value is greater than the range between min and max [which is the condition where truncation is most important].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with using truncateXAxis: true as the global default. Any objections @michael-s-molina ? I assume we can do this later without blocking the release, as it's a pretty minor change IMO..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection. We can cherry-pick the change for the next patch.

truncateYAxis: false,
yAxisBounds: [null, null],
xAxisLabelRotation: 0,
Expand Down
4 changes: 3 additions & 1 deletion superset-frontend/plugins/plugin-chart-echarts/src/Bubble/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
} from '@superset-ui/chart-controls';

import { DEFAULT_FORM_DATA } from './constants';
import { legendSection } from '../controls';
import { legendSection, truncateXAxis, xAxisBounds } from '../controls';

const { logAxis, truncateYAxis, yAxisBounds, xAxisLabelRotation, opacity } =
DEFAULT_FORM_DATA;
Expand Down Expand Up @@ -247,6 +247,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
17 changes: 11 additions & 6 deletions superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ import {
import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
import { defaultGrid } from '../defaults';
import { getLegendProps } from '../utils/series';
import { getLegendProps, getMinAndMaxFromBounds } from '../utils/series';
import { Refs } from '../types';
import { parseYAxisBound } from '../utils/controls';
import { parseAxisBound } from '../utils/controls';
import { getDefaultTooltip } from '../utils/tooltip';
import { getPadding } from '../Timeseries/transformers';
import { convertInteger } from '../utils/convertInteger';
Expand Down Expand Up @@ -84,13 +84,15 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
series: bubbleSeries,
xAxisLabel: bubbleXAxisTitle,
yAxisLabel: bubbleYAxisTitle,
xAxisBounds,
xAxisFormat,
yAxisFormat,
yAxisBounds,
logXAxis,
logYAxis,
xAxisTitleMargin,
yAxisTitleMargin,
truncateXAxis,
truncateYAxis,
xAxisLabelRotation,
yAxisLabelRotation,
Expand Down Expand Up @@ -141,7 +143,8 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
const yAxisFormatter = getNumberFormatter(yAxisFormat);
const tooltipSizeFormatter = getNumberFormatter(tooltipSizeFormat);

const [min, max] = yAxisBounds.map(parseYAxisBound);
const [xAxisMin, xAxisMax] = xAxisBounds.map(parseAxisBound);
const [yAxisMin, yAxisMax] = yAxisBounds.map(parseAxisBound);

const padding = getPadding(
showLegend,
Expand All @@ -155,6 +158,7 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
convertInteger(xAxisTitleMargin),
);

const xAxisType = logXAxis ? AxisType.log : AxisType.value;
const echartOptions: EChartsCoreOption = {
series,
xAxis: {
Expand All @@ -172,7 +176,8 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
fontWight: 'bolder',
},
nameGap: convertInteger(xAxisTitleMargin),
type: logXAxis ? AxisType.log : AxisType.value,
type: xAxisType,
...getMinAndMaxFromBounds(xAxisType, truncateXAxis, xAxisMin, xAxisMax),
},
yAxis: {
axisLabel: { formatter: yAxisFormatter },
Expand All @@ -189,8 +194,8 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
fontWight: 'bolder',
},
nameGap: convertInteger(yAxisTitleMargin),
min,
max,
min: yAxisMin,
max: yAxisMax,
type: logYAxis ? AxisType.log : AxisType.value,
},
legend: {
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import {
ForecastSeriesEnum,
Refs,
} from '../types';
import { parseYAxisBound } from '../utils/controls';
import { parseAxisBound } from '../utils/controls';
import {
getOverMaxHiddenFormatter,
dedupSeries,
Expand Down Expand Up @@ -345,9 +345,9 @@ export default function transformProps(
});

// yAxisBounds need to be parsed to replace incompatible values with undefined
let [min, max] = (yAxisBounds || []).map(parseYAxisBound);
let [min, max] = (yAxisBounds || []).map(parseAxisBound);
let [minSecondary, maxSecondary] = (yAxisBoundsSecondary || []).map(
parseYAxisBound,
parseAxisBound,
);

const array = ensureIsArray(chartProps.rawFormData?.time_compare);
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
richTooltipSection,
seriesOrderSection,
percentageThresholdControl,
truncateXAxis,
xAxisBounds,
} from '../../controls';
import { AreaChartStackControlOptions } from '../../constants';

Expand Down Expand Up @@ -241,6 +243,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
richTooltipSection,
seriesOrderSection,
showValueSection,
truncateXAxis,
xAxisBounds,
} from '../../../controls';

import { OrientationType } from '../../types';
Expand Down Expand Up @@ -224,6 +226,8 @@ function createAxisControl(axis: 'x' | 'y'): ControlSetRow[] {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ import {
richTooltipSection,
seriesOrderSection,
showValueSection,
truncateXAxis,
xAxisBounds,
} from '../../../controls';

const {
Expand Down Expand Up @@ -229,6 +231,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
4 changes: 4 additions & 0 deletions ...set-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
richTooltipSection,
seriesOrderSection,
showValueSection,
truncateXAxis,
xAxisBounds,
} from '../../../controls';

const {
Expand Down Expand Up @@ -173,6 +175,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
4 changes: 4 additions & 0 deletions ...-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
richTooltipSection,
seriesOrderSection,
showValueSectionWithoutStack,
truncateXAxis,
xAxisBounds,
} from '../../../controls';

const {
Expand Down Expand Up @@ -173,6 +175,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
richTooltipSection,
seriesOrderSection,
showValueSection,
truncateXAxis,
xAxisBounds,
} from '../../controls';

const {
Expand Down Expand Up @@ -223,6 +225,8 @@ const config: ControlPanelConfig = {
},
},
],
[truncateXAxis],
[xAxisBounds],
[
{
name: 'truncateYAxis',
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export const DEFAULT_FORM_DATA: EchartsTimeseriesFormData = {
seriesType: EchartsTimeseriesSeriesType.Line,
stack: false,
tooltipTimeFormat: 'smart_date',
truncateXAxis: true,
truncateYAxis: false,
yAxisBounds: [null, null],
zoomable: false,
Expand Down
28 changes: 19 additions & 9 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
} from './types';
import { DEFAULT_FORM_DATA } from './constants';
import { ForecastSeriesEnum, ForecastValue, Refs } from '../types';
import { parseYAxisBound } from '../utils/controls';
import { parseAxisBound } from '../utils/controls';
import {
calculateLowerLogTick,
dedupSeries,
Expand All @@ -64,6 +64,7 @@
getAxisType,
getColtypesMapping,
getLegendProps,
getMinAndMaxFromBounds,
} from '../utils/series';
import {
extractAnnotationLabels,
Expand Down Expand Up @@ -161,8 +162,10 @@
stack,
tooltipTimeFormat,
tooltipSortByMetric,
truncateXAxis,
truncateYAxis,
xAxis: xAxisOrig,
xAxisBounds,
xAxisLabelRotation,
xAxisSortSeries,
xAxisSortSeriesAscending,
Expand Down Expand Up @@ -388,15 +391,20 @@
}
});

// yAxisBounds need to be parsed to replace incompatible values with undefined
let [min, max] = (yAxisBounds || []).map(parseYAxisBound);
// axis bounds need to be parsed to replace incompatible values with undefined
const [xAxisMin, xAxisMax] = (xAxisBounds || []).map(parseAxisBound);
let [yAxisMin, yAxisMax] = (yAxisBounds || []).map(parseAxisBound);

// default to 0-100% range when doing row-level contribution chart
if ((contributionMode === 'row' || isAreaExpand) && stack) {
if (min === undefined) min = 0;
if (max === undefined) max = 1;
} else if (logAxis && min === undefined && minPositiveValue !== undefined) {
min = calculateLowerLogTick(minPositiveValue);
if (yAxisMin === undefined) yAxisMin = 0;
if (yAxisMax === undefined) yAxisMax = 1;
} else if (
logAxis &&
yAxisMin === undefined &&
minPositiveValue !== undefined
) {
yAxisMin = calculateLowerLogTick(minPositiveValue);

Check warning on line 407 in superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts#L407 

Added line #L407 was not covered by tests
}

const tooltipFormatter =
Expand Down Expand Up @@ -452,12 +460,14 @@
xAxisType === AxisType.time && timeGrainSqla
? TIMEGRAIN_TO_TIMESTAMP[timeGrainSqla]
: 0,
...getMinAndMaxFromBounds(xAxisType, truncateXAxis, xAxisMin, xAxisMax),
};

let yAxis: any = {
...defaultYAxis,
type: logAxis ? AxisType.log : AxisType.value,
min,
max,
min: yAxisMin,
max: yAxisMax,
minorTick: { show: true },
minorSplitLine: { show: minorSplitLine },
axisLabel: {
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,12 @@ export type EchartsTimeseriesFormData = QueryFormData & {
stack: StackType;
timeCompare?: string[];
tooltipTimeFormat?: string;
truncateXAxis: boolean;
truncateYAxis: boolean;
yAxisFormat?: string;
xAxisTimeFormat?: string;
timeGrainSqla?: TimeGranularity;
xAxisBounds: [number | undefined | null, number | undefined | null];
yAxisBounds: [number | undefined | null, number | undefined | null];
zoomable: boolean;
richTooltip: boolean;
Expand Down
31 changes: 31 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,34 @@
[sortSeriesType],
[sortSeriesAscending],
];

export const truncateXAxis: ControlSetItem = {
name: 'truncateXAxis',
config: {
type: 'CheckboxControl',
label: t('Truncate X Axis'),
default: DEFAULT_FORM_DATA.truncateXAxis,
renderTrigger: true,
description: t(
'Truncate X Axis. Can be overridden by specifying a min or max bound. Only applicable for numercal X axis.',
),
},
};

export const xAxisBounds: ControlSetItem = {
name: 'xAxisBounds',
config: {
type: 'BoundsControl',
label: t('X Axis Bounds'),
renderTrigger: true,
default: DEFAULT_FORM_DATA.xAxisBounds,
description: t(
'Bounds for numerical X axis. Not applicable for temporal or categorical axes. ' +
'When left empty, the bounds are dynamically defined based on the min/max of the data. ' +
"Note that this feature will only expand the axis range. It won't " +
"narrow the data's extent.",
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
Boolean(controls?.truncateXAxis?.value),

Check warning on line 279 in superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx#L279 

Added line #L279 was not covered by tests
},
};
2 changes: 1 addition & 1 deletion superset-frontend/plugins/plugin-chart-echarts/src/utils/controls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { validateNumber } from '@superset-ui/core';

// eslint-disable-next-line import/prefer-default-export
export function parseYAxisBound(
export function parseAxisBound(
bound?: string | number | null,
): number | undefined {
if (bound === undefined || bound === null || Number.isNaN(Number(bound))) {
Expand Down
14 changes: 14 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,3 +543,17 @@ export function calculateLowerLogTick(minPositiveValue: number) {
const logBase10 = Math.floor(Math.log10(minPositiveValue));
return Math.pow(10, logBase10);
}

export function getMinAndMaxFromBounds(
axisType: AxisType,
truncateAxis: boolean,
min?: number,
max?: number,
): { min: number | 'dataMin'; max: number | 'dataMax' } | {} {
return truncateAxis && axisType === AxisType.value
? {
min: min === undefined ? 'dataMin' : min,
max: max === undefined ? 'dataMax' : max,
}
: {};
}
1 change: 1 addition & 0 deletions superset-frontend/plugins/plugin-chart-echarts/test/Bubble/transformProps.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe('Bubble transformProps', () => {
expressionType: 'simple',
label: 'SUM(sales)',
},
xAxisBounds: [null, null],
yAxisBounds: [null, null],
};
const chartProps = new ChartProps({
Expand Down
Loading
Loading