codeant-ai-for-open-source[bot] commented on code in PR #38673:
URL: https://github.com/apache/superset/pull/38673#discussion_r3015215485
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -824,12 +824,21 @@ export default function transformProps(
isHorizontal,
);
+ // Reduce grid padding for small charts to maximize the drawing area.
+ // Keep enough top padding so the max label doesn't clip against the cell
border.
+ // Preserve bottom padding when zoomable, since getPadding() reserves space
for the dataZoom slider.
+ if (height < TIMESERIES_CONSTANTS.compactChartHeight) {
+ padding.top = Math.min(padding.top, 12);
+ if (!zoomable) {
Review Comment:
**Suggestion:** In compact mode the legend is hidden later, but `padding` is
still computed as if the legend is visible, so the chart can keep large
reserved margins (especially with left/right legends) and lose plot area.
Recompute compact padding with legend disabled so the drawing area actually
expands. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Compact charts waste space reserved for hidden legend.
- ⚠️ Reduced plot area in dense dashboard layouts.
```
</details>
```suggestion
const compactPadding = getPadding(
false,
legendOrientation,
addYAxisLabelOffset,
zoomable,
effectiveLegendMargin,
addXAxisLabelOffset,
yAxisTitlePosition,
convertInteger(yAxisTitleMargin),
convertInteger(xAxisTitleMargin),
isHorizontal,
);
Object.assign(padding, compactPadding);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an ECharts time-series chart that uses `transformProps`
(default export of
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`)
with
`showLegend = true` and a non-null `legendMargin`, and place it in a
dashboard cell whose
height is below `TIMESERIES_CONSTANTS.compactChartHeight` (compact mode).
2. In `transformProps`, `padding` is first computed via
`getPadding(showLegend,
legendOrientation, addYAxisLabelOffset, zoomable, effectiveLegendMargin,
addXAxisLabelOffset, ...)` (lines ~815–825), which reserves space for the
legend based on
`showLegend` and `legendOrientation`.
3. Immediately after, the compact-mode block (lines 831–835) only clamps
`padding.top` and
`padding.bottom` but does not recompute padding with `showLegend = false`,
while the
legend configuration later passes `isSmallChart ? false : showLegend` into
`getLegendProps` (lines ~1097–1105), effectively hiding the legend in
compact mode without
clearing the reserved padding.
4. Render the chart in the browser: the legend is not shown for compact
heights, but the
grid (`grid: { ...defaultGrid, ...padding }` near the end of
`transformProps`) still
includes space that had been reserved for the legend (especially on the side
where the
legend orientation is configured), reducing the actual plot area available
for the
time-series lines/bars.
```
</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:** 832:832
**Comment:**
*Logic Error: In compact mode the legend is hidden later, but `padding`
is still computed as if the legend is visible, so the chart can keep large
reserved margins (especially with left/right legends) and lose plot area.
Recompute compact padding with legend disabled so the drawing area actually
expands.
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=9ebabb1ed809295d57205dc0200516e34524859497e3c10d151760308ef0adc2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38673&comment_hash=9ebabb1ed809295d57205dc0200516e34524859497e3c10d151760308ef0adc2&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]
