codeant-ai-for-open-source[bot] commented on code in PR #38673:
URL: https://github.com/apache/superset/pull/38673#discussion_r2939444279
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -664,6 +664,13 @@ export default function transformProps(
isHorizontal,
);
+ // Reduce grid padding for compact charts to maximize the drawing area.
+ // Keep enough top padding so the max label doesn't clip against the cell
border.
+ if (height < TIMESERIES_CONSTANTS.compactChartHeight) {
+ padding.top = Math.min(padding.top, 12);
+ padding.bottom = Math.min(padding.bottom, 5);
+ }
Review Comment:
**Suggestion:** The compact-padding branch always shrinks `padding.bottom`
to 5px, even when zoom is enabled. `getPadding` intentionally reserves extra
bottom space for the dataZoom slider, so overriding it causes the zoom slider
and x-axis area to overlap in compact charts. Keep the bottom reduction only
for non-zoomable charts. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Zoom slider overlaps x-axis in compact Timeseries charts.
- ⚠️ Dense dashboard charts lose zoom usability and readability.
```
</details>
```suggestion
if (height < TIMESERIES_CONSTANTS.compactChartHeight) {
padding.top = Math.min(padding.top, 12);
if (!zoomable) {
padding.bottom = Math.min(padding.bottom, 5);
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any Timeseries chart type (plugin wires `transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:73`)
and enable
zoom from controls (`['zoomable']` at
`Timeseries/Regular/Line/controlPanel.tsx:138`).
2. Render the chart in a compact container (height < 100);
`transformProps()` reads
`height` from chart props at `Timeseries/transformProps.ts:122-124` and
enters compact
branch at `:669`.
3. In the same execution, `getPadding(..., zoomable, ...)` is called at
`Timeseries/transformProps.ts:654-665`; `getPadding()` reserves zoom bottom
space
(`gridOffsetBottomZoomable`) when zoomable at
`Timeseries/transformers.ts:745-747`.
4. That reserved bottom is immediately reduced to `5` at
`Timeseries/transformProps.ts:671`, while slider remains configured at
`dataZoom.bottom =
TIMESERIES_CONSTANTS.zoomBottom` (`Timeseries/transformProps.ts:984-990`,
constant at
`constants.ts:41`), causing slider/x-axis/grid overlap in compact zoomable
charts.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 669:672
**Comment:**
*Logic Error: The compact-padding branch always shrinks
`padding.bottom` to 5px, even when zoom is enabled. `getPadding` intentionally
reserves extra bottom space for the dataZoom slider, so overriding it causes
the zoom slider and x-axis area to overlap in compact charts. Keep the bottom
reduction only for non-zoomable charts.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38673&comment_hash=a937e327202b55522499c2bfc10b894009aba2a29b2d7f871ee89f2232d72a74&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38673&comment_hash=a937e327202b55522499c2bfc10b894009aba2a29b2d7f871ee89f2232d72a74&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -749,14 +756,30 @@ export default function transformProps(
),
};
+ // Adapt y-axis to chart height: three tiers based on available space.
+ // >= 100px: full axis with proportional tick count
+ // 60-99px: show only min/max labels, hide lines/ticks
+ // < 60px: hide all axis decorations, show line only
+ const isCompactChart = height < TIMESERIES_CONSTANTS.compactChartHeight;
+ const isMicroChart = height < TIMESERIES_CONSTANTS.microChartHeight;
+ const yAxisSplitNumber = isCompactChart
+ ? undefined
+ : Math.max(3, Math.floor(height /
TIMESERIES_CONSTANTS.yAxisTickHeightInterval));
Review Comment:
**Suggestion:** The compact tier is documented as "show only min/max
labels", but `splitNumber` is left undefined there, so ECharts can still
generate intermediate ticks and labels. Set `splitNumber` to 1 for compact
(non-micro) charts so only min/max labels are produced consistently. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Compact 60-99px charts still show extra y labels.
- ❌ Small dashboard cells become cluttered and harder to read.
```
</details>
```suggestion
const yAxisSplitNumber = isMicroChart
? undefined
: isCompactChart
? 1
: Math.max(
3,
Math.floor(height / TIMESERIES_CONSTANTS.yAxisTickHeightInterval),
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use any Timeseries chart path that invokes `transformProps`
(`Timeseries/index.ts:73`)
and render at height between compact and micro thresholds
(`compactChartHeight=100`,
`microChartHeight=60` in `constants.ts:51-52`).
2. `transformProps()` computes `isCompactChart=true`, `isMicroChart=false` at
`Timeseries/transformProps.ts:763-765`, matching the "60-99px" compact tier
comment
(`:760-762`).
3. Current code sets `yAxisSplitNumber` to `undefined` for all compact
charts at
`Timeseries/transformProps.ts:765-767`, and only conditionally applies
`splitNumber` when
defined at `:772`.
4. Because compact non-micro still shows labels
(`axisLabel.show/showMinLabel/showMaxLabel` at `:779-781`) but leaves split
count
uncontrolled, ECharts auto-generates intermediate ticks/labels, violating
intended
"min/max only" compact behavior and reintroducing label crowding.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 765:767
**Comment:**
*Logic Error: The compact tier is documented as "show only min/max
labels", but `splitNumber` is left undefined there, so ECharts can still
generate intermediate ticks and labels. Set `splitNumber` to 1 for compact
(non-micro) charts so only min/max labels are produced consistently.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38673&comment_hash=c8b6863beddc5bf2d53716b3711d25deb12245fe324842c8b295e484d8e56d67&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38673&comment_hash=c8b6863beddc5bf2d53716b3711d25deb12245fe324842c8b295e484d8e56d67&reaction=dislike'>👎</a>
--
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]
