sadpandajoe commented on code in PR #44147:
URL: https://github.com/apache/superset/pull/44147#discussion_r4148409513
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -600,6 +610,10 @@ export default function transformProps(
legendOrientation,
showLegend,
theme,
+ false, // zoomable — Pie charts do not use the zoom control
+ undefined, // legendState — not tracked per-item in Pie
Review Comment:
`getLegendProps` takes 8 parameters ending at `horizontalLegendWidth`, but
this call passes 10. `legendState` (destructured from `chartProps` above) ends
up two slots past the last parameter and is silently dropped, so this line's
`undefined` is what the function actually receives for `legendState`. A user
who hides a slice via the legend will see it reselected after any re-render,
since `legend.selected` always resolves to `{}`. Should `legendState` be passed
here instead of `undefined`, with the trailing `false, legendState` on the next
two lines removed?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -554,10 +556,18 @@ export default function transformProps(
position: 'outer',
alignTo: 'none',
bleedMargin: 5,
+ ...(labelMaxWidth > 0 && {
+ width: labelMaxWidth,
+ overflow: labelOverflow,
Review Comment:
This always includes `overflow: labelOverflow` (literally the string
`'none'`) whenever `labelMaxWidth > 0`, but this PR's own
`test/Pie/transformProps.test.ts` assertion `sets width but omits overflow
field when labelOverflow is none` expects the `overflow` key to be absent in
that case, and currently fails against this line (and the matching inner-label
spot just below). Should this omit `overflow` when `labelOverflow === 'none'`
to match the test, or should the test's expectation change to match this
behavior?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -895,6 +912,55 @@ export function getLegendProps(
const getLegendWidth = (paddingWidth: number) =>
Math.max(paddingWidth - MARGIN_GUTTER, MIN_LEGEND_WIDTH);
+ /**
+ * Returns a legend tooltip config that:
+ * 1. Only appears when the label is actually truncated (name wider than
maxTextWidth)
+ * 2. Positions the tooltip ABOVE the legend item to avoid overlapping the
chart
+ */
+ const makeLegendTooltip = (maxTextWidth: number): any => ({
+ show: true,
+ confine: false, // allow tooltip to render above the canvas boundary
+ position: (
+ _pos: [number, number],
+ _params: unknown,
+ _el: unknown,
+ elRect: { x: number; y: number; width: number; height: number },
+ size: { contentSize: [number, number]; viewSize: [number, number] },
+ ) => {
+ const tooltipWidth = size.contentSize[0];
+ const tooltipHeight = size.contentSize[1];
+ // Center horizontally over the hovered legend item
+ const x = Math.max(
+ Math.min(
+ elRect.x + elRect.width / 2 - tooltipWidth / 2,
+ size.viewSize[0] - tooltipWidth,
+ ),
+ 0,
+ );
+ // Place above the legend item; negative y appears above canvas with
confine:false
+ const y = elRect.y - tooltipHeight - 8;
+ return [x, y];
+ },
+ formatter: (params: { name: string }) => {
Review Comment:
This arrow function's block body contains only a comment and a single
return, which fails this repo's `arrow-body-style` lint rule; `lint-frontend`
and `pre-commit` are both currently red on exactly this line. Moving the
comment above the arrow and converting to an implicit-return expression
(`measureTextWidth(params.name, theme) > maxTextWidth ? escape(params.name) :
''`) would clear both checks.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -895,6 +912,55 @@ export function getLegendProps(
const getLegendWidth = (paddingWidth: number) =>
Math.max(paddingWidth - MARGIN_GUTTER, MIN_LEGEND_WIDTH);
+ /**
+ * Returns a legend tooltip config that:
+ * 1. Only appears when the label is actually truncated (name wider than
maxTextWidth)
+ * 2. Positions the tooltip ABOVE the legend item to avoid overlapping the
chart
+ */
+ const makeLegendTooltip = (maxTextWidth: number): any => ({
Review Comment:
This tooltip only appears on mouse hover, with no keyboard-focus or touch
equivalent, so a keyboard or screen-reader user who hits a truncated legend
item has no way to recover the untruncated name that this feature exists to
surface for everyone else. Is a non-hover affordance (e.g. an
accessible-name/title equivalent) in scope here, or tracked separately?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts:
##########
@@ -618,6 +618,63 @@ describe('legend sorting', () => {
});
});
+describe('Pie label max width and overflow', () => {
+ const makeChartProps = (
+ labelMaxWidth: number,
+ labelOverflow: string,
+ labelsOutside = true,
+ ) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum__num',
+ groupby: ['category'],
+ viz_type: 'pie',
+ label_max_width: labelMaxWidth,
+ label_overflow: labelOverflow,
+ labels_outside: labelsOutside,
+ } as SqlaFormData,
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A very long category name indeed', sum__num: 10 },
+ { category: 'Another very long category name', sum__num: 20 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ }) as EchartsPieChartProps;
+
+ const getLabel = (props: EchartsPieChartProps) =>
Review Comment:
This `describe('Pie label max width and overflow')` block only ever reads
`series[0].label`, so none of its four tests exercise the
`horizontalLegendWidth` passthrough into `getLegendProps` that this PR also
adds for the legend truncation/tooltip half of this fix. Removing that argument
from `transformProps.ts` would leave every test here green while long legend
names could clip again with no recovery tooltip. Could a case here assert on
`transformProps(...).echartOptions.legend`'s `textStyle`/`tooltip` for a
long-name fixture?
--
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]
