codeant-ai-for-open-source[bot] commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3006206515
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -947,6 +947,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.min(
Review Comment:
**Suggestion:** The responsive zoom slider offset uses `Math.min` instead of
`Math.max`, which reduces the bottom offset below the intended minimum on short
charts and can cause the slider to be closer to (or clipped by) the chart edge
instead of preserving a minimum spacing while growing with height. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ECharts Time-series zoom slider too close to bottom.
- ⚠️ Zoom slider can be partially clipped in short panels.
- ⚠️ Degraded usability of zoom control in Explore.
```
</details>
```suggestion
const dynamicZoomBottom = Math.max(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render an ECharts Time-series chart using the
`EchartsTimeseriesChartPlugin` defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:34-74`,
which
wires `transformProps` from `./transformProps` into the plugin.
2. Ensure the chart is configured with zoom enabled so that `zoomable` is
true and passed
into `transformProps` (see the `zoomable` field in the
`EchartsTimeseriesFormData`
destructuring in `Timeseries/transformProps.ts:217-276` and the `dataZoom`
configuration
at `Timeseries/transformProps.ts:1108-1129`).
3. Place the chart in a short container (for example a small dashboard row
or a resized
Storybook demo) so that the `height` prop passed into `transformProps` (see
`Timeseries/transformProps.ts:181-197` and
`Timeseries/EchartsTimeseries.tsx:41-61`) is
relatively small, e.g. around 200 pixels.
4. When `transformProps` runs, it computes `dynamicZoomBottom` as
`Math.min(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))` at
`Timeseries/transformProps.ts:950-953`, with
`TIMESERIES_CONSTANTS.zoomBottom` defined as
`30` in `plugins/plugin-chart-echarts/src/constants.ts:33-42`. For `height =
200`, this
yields `Math.min(30, 16) = 16`, which is *less* than the intended minimum
spacing of 30.
This value is then used as `bottom: dynamicZoomBottom` for the slider
dataZoom component
at `Timeseries/transformProps.ts:1110-1115`, causing the slider to render
closer to (or
partially clipped by) the bottom edge of the chart container in short charts
instead of
preserving a minimum spacing. Using `Math.max` as described in the PR text
would keep the
offset at least `zoomBottom` and avoid this regression.
```
</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:** 950:950
**Comment:**
*Logic Error: The responsive zoom slider offset uses `Math.min` instead
of `Math.max`, which reduces the bottom offset below the intended minimum on
short charts and can cause the slider to be closer to (or clipped by) the chart
edge instead of preserving a minimum spacing while growing with height.
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=22126d0fb8575f2bfe2edf0286323fc89622e28cff737d6c6b7ef7add4260dd8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=22126d0fb8575f2bfe2edf0286323fc89622e28cff737d6c6b7ef7add4260dd8&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]
