EnxDev commented on code in PR #45520:
URL: https://github.com/apache/superset/pull/45520#discussion_r4328494144
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/TimeseriesLegend.tsx:
##########
@@ -21,16 +21,17 @@ import { styled } from '@apache-superset/core/theme';
import { t } from '@apache-superset/core/translation';
import type { TimeseriesCustomLegend } from './types';
-const LegendViewport = styled.div<{ maxHeight: number }>`
- ${({ maxHeight, theme }) => `
+const LegendViewport = styled.div<{ maxHeight: number; maxWidth?: number }>`
+ ${({ maxHeight, maxWidth, theme }) => `
box-sizing: border-box;
color: ${theme.colorText};
font-size: ${theme.fontSizeSM}px;
- max-height: ${maxHeight}px;
+ max-height: ${maxHeight > 0 ? `${maxHeight}px` : 'none'};
+ ${maxWidth !== undefined && maxWidth > 0 ? `max-width: ${maxWidth}px;` :
''}
overflow-x: hidden;
overflow-y: auto;
padding: ${theme.sizeUnit}px ${theme.sizeUnit * 2}px;
- width: 100%;
+ width: ${maxWidth !== undefined && maxWidth > 0 ? `${maxWidth}px` :
'100%'};
Review Comment:
This pins the legend to exactly 40% of the chart width for Left/Right, so a
chart with two short series like "A" and "B" still gives up 40% of its plot to
a mostly empty column. The native legend sized to its content, so simple charts
get visibly worse.
`max-width` on line 30 already does the capping. With `auto` here the column
should size to the longest label and stop at the cap:
```suggestion
width: ${maxWidth !== undefined && maxWidth > 0 ? 'auto' : '100%'};
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/TimeseriesLegend.tsx:
##########
@@ -134,18 +150,32 @@ export type TimeseriesLegendProps =
TimeseriesCustomLegend & {
function TimeseriesLegend({
items,
maxHeight,
+ maxWidth,
onAll,
onHover,
onInverse,
onToggle,
+ orientation,
showSelectors,
}: TimeseriesLegendProps) {
- if (items.length === 0 || maxHeight <= 0) {
+ const isVertical =
+ orientation === 'left' || orientation === 'right';
Review Comment:
oxfmt rewrites this and `EchartsTimeseries.tsx:839`, which is what's failing
pre-commit.
Running `pre-commit run` on the branch fixes both:
```suggestion
const isVertical = orientation === 'left' || orientation === 'right';
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1227,7 +1227,9 @@ export default function transformProps(
isLegendVisible &&
legendType === LegendType.Plain &&
(legendOrientation === LegendOrientation.Top ||
- legendOrientation === LegendOrientation.Bottom);
+ legendOrientation === LegendOrientation.Bottom ||
+ legendOrientation === LegendOrientation.Left ||
+ legendOrientation === LegendOrientation.Right);
Review Comment:
`keeps plain/left legends on the native ECharts path` and its `/right` twin
in `test/Timeseries/transformProps.test.ts:1280` still assert the old behavior,
so they're what's red in sharded-jest-tests (2).
Could you flip them to expect the custom legend, and add coverage for the
new pieces too? `getTimeseriesLegendMaxWidth` and a `TimeseriesLegend` render
with `orientation: 'right'` (column layout, truncation) would do it. Nothing on
the vertical path is tested yet.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/TimeseriesLegend.tsx:
##########
@@ -183,7 +213,7 @@ function TimeseriesLegend({
color={item.color}
selected={item.selected}
/>
- <Label>{item.name}</Label>
+ <Label truncate={isVertical}>{item.name}</Label>
Review Comment:
Once a label gets cut to an ellipsis there's no way to read the full series
name, and long groupby names are exactly the case this PR is fixing.
A `title` gives it back on hover:
```suggestion
<Label
title={isVertical ? item.name : undefined}
truncate={isVertical}
>
{item.name}
</Label>
```
--
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]
