codeant-ai-for-open-source[bot] commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r2992716825
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -865,6 +865,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.max(
Review Comment:
**Suggestion:** The `dynamicZoomBottom` calculation uses `Math.max`, which
only increases the bottom offset for taller charts and leaves it fixed for
smaller ones, so the zoom slider does not actually adapt to short chart heights
as intended and can remain clipped; using `Math.min` instead makes the offset
shrink with chart height while capping it at the existing constant spacing.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Short-height ECharts timeseries charts keep non-responsive zoom slider
offset.
- ⚠️ Zoomable control still clipped in small Explore panels.
```
</details>
```suggestion
const dynamicZoomBottom = Math.min(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an ECharts Timeseries chart in Explore or a dashboard; this uses
`EchartsTimeseriesChartPlugin` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:3-11`,
which in
turn uses `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`.
2. Ensure the **Zoomable** control is enabled so `zoomable` is true and a
`dataZoom`
slider is added (see `transformProps.ts:1020-1034`, where `dataZoom` is
configured when
`zoomable` is truthy).
3. Use a short chart height (e.g., small dashboard row or a resized Explore
panel), so the
`height` passed via `EchartsTimeseriesChartProps` into `transformProps` is
below ~375px;
`transformProps.ts:69-72` computes `dynamicZoomBottom =
Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))`.
4. Observe from the code that for all such short heights `Math.floor(height
* 0.08) <=
TIMESERIES_CONSTANTS.zoomBottom` (`zoomBottom` is 30 per
`superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:33-41`), so
`dynamicZoomBottom` always resolves to 30; therefore the slider `bottom`
value at
`transformProps.ts:1023-1027` is identical to the pre-PR fixed offset and
does not change
with height in the exact "small chart" range the PR description claims to
fix, meaning the
zoom slider layout issues for short charts are not actually addressed.
```
</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:** 868:868
**Comment:**
*Logic Error: The `dynamicZoomBottom` calculation uses `Math.max`,
which only increases the bottom offset for taller charts and leaves it fixed
for smaller ones, so the zoom slider does not actually adapt to short chart
heights as intended and can remain clipped; using `Math.min` instead makes the
offset shrink with chart height while capping it at the existing constant
spacing.
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%2F38657&comment_hash=132544f99f34a8d7b0d08389801ab2a68b7967cfb0e387037ee3a66e77537927&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=132544f99f34a8d7b0d08389801ab2a68b7967cfb0e387037ee3a66e77537927&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]
