codeant-ai-for-open-source[bot] commented on code in PR #38675:
URL: https://github.com/apache/superset/pull/38675#discussion_r2982205397
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -692,15 +681,86 @@ export default function transformProps(
}
return [];
})()
- : // Otherwise show original series names
- rawSeries
+ : rawSeries
.filter(
entry =>
extractForecastSeriesContext(entry.name || '').type ===
ForecastSeriesEnum.Observation,
)
.map(entry => entry.name || '')
.concat(extractAnnotationLabels(annotationLayers));
+ const addYAxisLabelOffset =
+ !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisLabelOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
+
+ const sortedLegendData = [...legendData].sort((a: string, b: string) => {
+ if (!legendSort) return 0;
+ return legendSort === 'asc' ? a.localeCompare(b) : b.localeCompare(a);
+ });
+ const colorByPrimaryAxisLegendData = legendData.map(name => ({
+ name,
+ icon: 'roundRect',
+ }));
+ const getLegendLayout = (candidateLegendMargin?: string | number | null) => {
+ const padding = getPadding(
+ showLegend,
+ legendOrientation,
+ addYAxisLabelOffset,
+ zoomable,
+ candidateLegendMargin,
+ addXAxisLabelOffset,
+ yAxisTitlePosition,
+ convertInteger(yAxisTitleMargin),
+ convertInteger(xAxisTitleMargin),
+ isHorizontal,
+ );
+
+ return resolveLegendLayout({
+ availableWidth:
+ legendOrientation === LegendOrientation.Top ||
+ legendOrientation === LegendOrientation.Bottom
+ ? getHorizontalLegendAvailableWidth({
+ chartWidth: width,
+ orientation: legendOrientation,
+ padding,
+ zoomable,
+ })
+ : undefined,
+ chartHeight: height,
+ chartWidth: width,
+ legendItems:
+ colorByPrimaryAxis && groupBy.length === 0
+ ? colorByPrimaryAxisLegendData
+ : sortedLegendData,
+ legendMargin: candidateLegendMargin,
+ orientation: legendOrientation,
+ show: showLegend,
+ showSelectors: !(colorByPrimaryAxis && groupBy.length === 0),
+ theme,
+ type: legendType,
+ });
+ };
+ const initialLegendLayout = getLegendLayout(legendMargin);
+ const legendLayout =
+ isHorizontal &&
+ legendOrientation === LegendOrientation.Bottom &&
+ initialLegendLayout.effectiveLegendType === LegendType.Plain
+ ? getLegendLayout(initialLegendLayout.effectiveLegendMargin)
+ : initialLegendLayout;
+ const { effectiveLegendMargin, effectiveLegendType } = legendLayout;
Review Comment:
**Suggestion:** The second-pass legend recomputation can switch from plain
to scroll, but it keeps the expanded plain-legend margin from the first pass.
This leaves excessive chart padding and can shrink the plotting area
unnecessarily. When fallback to scroll happens, reset the margin to the
original configured margin instead of reusing the expanded plain margin. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Horizontal bottom legends can waste chart plotting space.
- ⚠️ Dashboard charts show unnecessary extra legend padding.
```
</details>
```suggestion
const effectiveLegendMargin =
isHorizontal &&
legendOrientation === LegendOrientation.Bottom &&
legendLayout.effectiveLegendType === LegendType.Scroll
? legendMargin
: legendLayout.effectiveLegendMargin;
const { effectiveLegendType } = legendLayout;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Timeseries plugin chart (entrypoint registers `transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:73`),
with
horizontal orientation, bottom legend, and plain legend.
2. In `transformProps` (`.../Timeseries/transformProps.ts:745-750`), the
second pass runs
for this exact condition and calls
`getLegendLayout(initialLegendLayout.effectiveLegendMargin)`.
3. In first pass, plain legends can increase margin (`Math.max(currentMargin,
requiredMargin)`) via `getHorizontalPlainLegendLayout` at
`.../utils/series.ts:249-291`.
4. That increased margin feeds `getPadding`
(`.../Timeseries/transformProps.ts:706-717`),
which for horizontal+bottom increases `padding.left`
(`.../utils/series.ts:892-901`),
reducing available width used by `getHorizontalLegendAvailableWidth`
(`.../utils/series.ts:209-227`).
5. Second pass can therefore flip to scroll (`effectiveType: Scroll` from
`.../utils/series.ts:281-286`), but `resolveLegendLayout` returns
`effectiveLegendMargin`
as the passed-in candidate margin when no scroll margin exists
(`.../utils/legendLayout.ts:52`).
6. Final padding still uses that inflated margin
(`.../Timeseries/transformProps.ts:752-763`), shrinking plot area even
though legend is
now scroll.
```
</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:** 751:751
**Comment:**
*Logic Error: The second-pass legend recomputation can switch from
plain to scroll, but it keeps the expanded plain-legend margin from the first
pass. This leaves excessive chart padding and can shrink the plotting area
unnecessarily. When fallback to scroll happens, reset the margin to the
original configured margin instead of reusing the expanded plain margin.
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%2F38675&comment_hash=f5335ac8e396484adbe928bb83dc6da8010980e5127feab387ff54f8286dcdb3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=f5335ac8e396484adbe928bb83dc6da8010980e5127feab387ff54f8286dcdb3&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]
