rusackas commented on code in PR #42300:
URL: https://github.com/apache/superset/pull/42300#discussion_r3685631623
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
+ return clamped;
+ };
+ const clampPoint = (point: AxisPoint): AxisPoint => {
+ const newPoint = [...point];
+ newPoint[valueIndex] = clampAxisValue(newPoint[valueIndex]);
+ return newPoint;
+ };
+ series.forEach(s => {
+ if (!Array.isArray(s.data)) return;
Review Comment:
Good catch, fixed. Annotation series were getting pushed into `series`
before the clamp loop ran, so their configured values were getting rewritten to
the axis bounds along with the real observation data. Snapshotted the series
count before annotations get appended and skip clamping anything past that
index now.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
Review Comment:
I don't think this changes behavior here. The clamp only touches a value
that individually exceeds the bound, so in your 8+8/max=10 example neither
segment gets touched, same as before this PR. ECharts clips bar rendering to
the grid area by default regardless of this, so an overflowing stack was
already visually truncated at the boundary, not dropped. And the
auto-calculated bounds path already accounts for the stacked total via
`stackedTotalMax`.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
+ return clamped;
+ };
+ const clampPoint = (point: AxisPoint): AxisPoint => {
+ const newPoint = [...point];
+ newPoint[valueIndex] = clampAxisValue(newPoint[valueIndex]);
+ return newPoint;
+ };
+ series.forEach(s => {
+ if (!Array.isArray(s.data)) return;
+ const clampedData = (
+ s.data as (AxisPoint | Record<string, unknown>)[]
+ ).map(point => {
+ if (Array.isArray(point)) {
+ return clampPoint(point);
+ }
+ // Some series paths (e.g. colorByPrimaryAxis, or negative bar
+ // label positioning) wrap the tuple in an object of the shape
+ // `{ value: [x, y], ... }` instead of passing the tuple
+ // directly; clamp the wrapped tuple in place so those points
+ // aren't skipped and left to be dropped by ECharts axis clipping.
+ if (
+ point &&
+ typeof point === 'object' &&
+ Array.isArray((point as { value?: unknown }).value)
+ ) {
+ return {
+ ...point,
+ value: clampPoint((point as { value: AxisPoint }).value),
+ };
+ }
+ return point;
+ });
+ s.data = clampedData as typeof s.data;
Review Comment:
Fair point, this does mutate the same data ECharts reads for tooltips and
labels, so a clamped point's tooltip shows the truncated value instead of the
real observation. Keeping the original value available to the formatters while
only clamping the render position would be the right fix, but that touches how
values flow through several of these formatters and I'd rather think it through
than bolt something on here.
--
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]
