bito-code-review[bot] commented on code in PR #38675:
URL: https://github.com/apache/superset/pull/38675#discussion_r2975608599
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -55,6 +55,364 @@ function isDefined<T>(value: T | undefined | null): boolean
{
return value !== undefined && value !== null;
}
+const DEFAULT_LEGEND_ITEM_GAP = 10;
+const DEFAULT_LEGEND_ICON_WIDTH = 25;
+const LEGEND_ICON_LABEL_GAP = 5;
+const LEGEND_HORIZONTAL_SIDE_GUTTER = 16;
+const LEGEND_HORIZONTAL_ROW_HEIGHT = 24;
+const LEGEND_HORIZONTAL_MAX_ROWS = 2;
+const LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO = 0.25;
+const LEGEND_VERTICAL_SIDE_GUTTER = 16;
+const LEGEND_VERTICAL_ROW_HEIGHT = 24;
+const LEGEND_VERTICAL_MAX_WIDTH_RATIO = 0.4;
+const LEGEND_SELECTOR_GAP = 10;
+const LEGEND_MARGIN_GUTTER = 45;
+// ECharts does not expose pre-render measurements for plain legends, so these
+// values intentionally overestimate selector space to avoid clipping.
+const ESTIMATED_LEGEND_SELECTOR_WIDTH = 112;
+const LEGEND_TEXT_WIDTH_CACHE = new Map<string, number>();
+
+type LegendDataItem =
+ | string
+ | number
+ | null
+ | undefined
+ | { name?: string | number | null };
+
+export type LegendLayoutResult = {
+ effectiveMargin?: number;
+ effectiveType: LegendType;
+};
+
+const SCROLL_LEGEND_LAYOUT: LegendLayoutResult = {
+ effectiveType: LegendType.Scroll,
+};
+
+function getLegendLabel(item: LegendDataItem): string {
+ if (typeof item === 'string' || typeof item === 'number') {
+ return String(item);
+ }
+
+ if (item?.name === undefined || item.name === null) {
+ return '';
+ }
+
+ return String(item.name);
+}
+
+function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+ const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
+ const cachedWidth = LEGEND_TEXT_WIDTH_CACHE.get(cacheKey);
+ if (cachedWidth !== undefined) {
+ return cachedWidth;
+ }
+
+ let width = text.length * theme.fontSizeSM * 0.62;
+
+ if (typeof document !== 'undefined') {
+ const canvas = document.createElement('canvas');
+ const context = canvas.getContext('2d');
+ if (context) {
+ context.font = `${theme.fontSizeSM}px ${theme.fontFamily}`;
+ ({ width } = context.measureText(text));
+ }
+ }
+
+ LEGEND_TEXT_WIDTH_CACHE.set(cacheKey, width);
+ return width;
+}
+
+function hasLegendLabel(item: LegendDataItem): boolean {
+ if (item === null || item === undefined) {
+ return false;
+ }
+
+ if (typeof item === 'object') {
+ return item.name !== null && item.name !== undefined;
+ }
+
+ return true;
+}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Logic error: includes empty labels</b></div>
<div id="fix">
The new hasLegendLabel function includes LegendDataItem values that result
in empty labels (e.g., empty strings or objects with empty name), unlike the
original code which filtered them out. This could lead to empty legend items
appearing in charts. Consider adjusting hasLegendLabel to return false for such
cases.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
function hasLegendLabel(item: LegendDataItem): boolean {
if (item === null || item === undefined) {
return false;
}
if (typeof item === 'object') {
return item.name !== null && item.name !== undefined &&
String(item.name) !== '';
}
return String(item) !== '';
}
````
</div>
</details>
</div>
<small><i>Code Review Run #b6660b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
