Copilot commented on code in PR #36306:
URL: https://github.com/apache/superset/pull/36306#discussion_r2578222119
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -433,69 +433,47 @@ export function getLegendProps(
type: LegendType,
orientation: LegendOrientation,
show: boolean,
- theme: SupersetTheme,
+ theme?: SupersetTheme,
Review Comment:
The `theme` parameter was changed from required to optional, but theme
properties are no longer being applied to the legend. This removes the
`selector` and `selectorLabel` configuration that provided styled toggle
buttons ('all', 'inverse') in the legend with proper theming (font family, font
size, colors, borders).
Either the theme parameter should be removed entirely if these properties
are no longer needed, or they should be restored:
```typescript
selector: ['all', 'inverse'],
selectorLabel: {
fontFamily: theme.fontFamily,
fontSize: theme.fontSizeSM,
color: theme.colorText,
borderColor: theme.colorBorder,
},
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -433,69 +433,47 @@ export function getLegendProps(
type: LegendType,
orientation: LegendOrientation,
show: boolean,
- theme: SupersetTheme,
+ theme?: SupersetTheme,
zoomable = false,
legendState?: LegendState,
Review Comment:
The `legendState` parameter is accepted but never used in the function body.
This breaks legend state persistence - when users toggle legend items on/off,
their selections won't be preserved. The `selected` property should be set in
the legend object:
```typescript
const legend: LegendComponentOption | LegendComponentOption[] = {
orient: isHorizontal ? 'horizontal' : 'vertical',
show,
type: effectiveType,
selected: legendState,
};
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -51,16 +51,7 @@ import {
import { defaultLegendPadding } from '../../src/defaults';
import { NULL_STRING } from '../../src/constants';
-const expectedThemeProps = {
- selector: ['all', 'inverse'],
- selected: undefined,
- selectorLabel: {
- fontFamily: theme.fontFamily,
- fontSize: theme.fontSizeSM,
- color: theme.colorText,
- borderColor: theme.colorBorder,
- },
-};
+const expectedThemeProps = {};
Review Comment:
The `expectedThemeProps` object was changed from containing theme-related
legend properties to an empty object. This means the tests are no longer
validating that theme properties (`selector`, `selectorLabel`) are correctly
applied to legends.
If these properties are intentionally removed from the implementation (as
seen in series.ts), the tests should be updated to explicitly verify they are
NOT present, rather than spreading an empty object. If they should be present,
the tests need to be reverted to validate them properly.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -433,69 +433,47 @@ export function getLegendProps(
type: LegendType,
orientation: LegendOrientation,
show: boolean,
- theme: SupersetTheme,
+ theme?: SupersetTheme,
zoomable = false,
legendState?: LegendState,
padding?: LegendPaddingType,
): LegendComponentOption | LegendComponentOption[] {
+ const isHorizontal =
+ orientation === LegendOrientation.Top ||
+ orientation === LegendOrientation.Bottom;
+
+ // If user explicitly selected scroll, keep it.
+ // Otherwise, for horizontal legends (top/bottom) default to scroll
+ // so long legends don't overlap the chart.
+ const effectiveType =
+ type === LegendType.Scroll || !isHorizontal ? type : LegendType.Scroll;
+
const legend: LegendComponentOption | LegendComponentOption[] = {
Review Comment:
[nitpick] The `legend` variable is declared with type `LegendComponentOption
| LegendComponentOption[]`, but it's always initialized as a single object and
never as an array. This requires unnecessary type casting throughout the switch
statement.
Consider either:
1. Changing the type to just `LegendComponentOption` if arrays are not needed
2. Or keeping the union type but initializing `legend` with explicit type
assertion: `const legend = { ... } as LegendComponentOption;`
This would eliminate the need for repeated type casting on lines 464, 469,
473, and 474.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -433,69 +433,47 @@ export function getLegendProps(
type: LegendType,
orientation: LegendOrientation,
show: boolean,
- theme: SupersetTheme,
+ theme?: SupersetTheme,
zoomable = false,
legendState?: LegendState,
padding?: LegendPaddingType,
Review Comment:
The `padding` parameter is accepted but never used in the function body.
This removes important functionality where legend text was truncated based on
available space for left/right orientations, and legend positioning was
adjusted based on padding for top/bottom orientations.
For left/right orientations, the original implementation set:
```typescript
legend.textStyle = {
overflow: 'truncate',
width: getLegendWidth(padding.left), // or padding.right
};
```
For top/bottom orientations, it set:
```typescript
legend.left = padding.left;
```
Either this parameter should be removed from the function signature, or the
padding logic should be restored.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -433,69 +433,47 @@ export function getLegendProps(
type: LegendType,
orientation: LegendOrientation,
show: boolean,
- theme: SupersetTheme,
+ theme?: SupersetTheme,
zoomable = false,
legendState?: LegendState,
padding?: LegendPaddingType,
): LegendComponentOption | LegendComponentOption[] {
+ const isHorizontal =
+ orientation === LegendOrientation.Top ||
+ orientation === LegendOrientation.Bottom;
+
+ // If user explicitly selected scroll, keep it.
+ // Otherwise, for horizontal legends (top/bottom) default to scroll
+ // so long legends don't overlap the chart.
+ const effectiveType =
+ type === LegendType.Scroll || !isHorizontal ? type : LegendType.Scroll;
+
const legend: LegendComponentOption | LegendComponentOption[] = {
- orient: [LegendOrientation.Top, LegendOrientation.Bottom].includes(
- orientation,
- )
- ? 'horizontal'
- : 'vertical',
+ orient: isHorizontal ? 'horizontal' : 'vertical',
show,
- type,
- selected: legendState,
- selector: ['all', 'inverse'],
- selectorLabel: {
- fontFamily: theme.fontFamily,
- fontSize: theme.fontSizeSM,
- color: theme.colorText,
- borderColor: theme.colorBorder,
- },
+ type: effectiveType,
};
- const MIN_LEGEND_WIDTH = 0;
- const MARGIN_GUTTER = 45;
- const getLegendWidth = (paddingWidth: number) =>
- Math.max(paddingWidth - MARGIN_GUTTER, MIN_LEGEND_WIDTH);
switch (orientation) {
case LegendOrientation.Left:
legend.left = 0;
- if (padding?.left) {
- legend.textStyle = {
- overflow: 'truncate',
- width: getLegendWidth(padding.left),
- };
- }
break;
case LegendOrientation.Right:
legend.right = 0;
- legend.top = zoomable ? TIMESERIES_CONSTANTS.legendRightTopOffset : 0;
- if (padding?.right) {
- legend.textStyle = {
- overflow: 'truncate',
- width: getLegendWidth(padding.right),
- };
- }
+ // keep existing offset behavior for zoomable charts
+ (legend as LegendComponentOption).top = zoomable
+ ? TIMESERIES_CONSTANTS.legendRightTopOffset
+ : 0;
break;
case LegendOrientation.Bottom:
- legend.bottom = 0;
- if (padding?.left) {
- legend.left = padding.left;
- }
+ (legend as LegendComponentOption).bottom = 0;
break;
case LegendOrientation.Top:
- legend.top = 0;
- legend.right = zoomable ? TIMESERIES_CONSTANTS.legendTopRightOffset : 0;
- if (padding?.left) {
- legend.left = padding.left;
- }
- break;
default:
Review Comment:
[nitpick] The `LegendOrientation.Top` case falls through to the `default`
case. While this works correctly, it would be clearer and more maintainable to
either:
1. Remove the explicit `case LegendOrientation.Top:` line and just handle it
in the default
2. Or add a comment explaining the intentional fall-through
3. Or combine them: `case LegendOrientation.Top: default:`
This makes the intent clearer that Top and the default case are handled
identically.
--
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]
