sadpandajoe commented on code in PR #44147:
URL: https://github.com/apache/superset/pull/44147#discussion_r4130843588
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -546,10 +548,18 @@ export default function transformProps(
position: 'outer',
alignTo: 'none',
bleedMargin: 5,
+ ...(labelMaxWidth > 0 && {
+ width: labelMaxWidth,
+ ...(labelOverflow !== 'none' && { overflow: labelOverflow }),
Review Comment:
Selecting "None" for Label Overflow still truncates the label with an
ellipsis. When `labelOverflow === 'none'`, the `overflow` key is omitted here
(and at the matching inner-label spot on line 561), so ECharts falls back to
its own Pie label default, which is `overflow: 'truncate'`, not "no
truncation". A user who sets a Label Max Width and leaves Overflow on its
default ("None") gets silently truncated labels even though the control implies
they opted out.
Can `overflow: labelOverflow` be passed unconditionally in both branches
instead of only when it isn't `'none'`, so `'none'` is sent explicitly and
actually disables truncation?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -853,6 +870,51 @@ 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.min(
Review Comment:
When the full (untruncated) name is wider than the chart itself,
`size.viewSize[0] - tooltipWidth` goes negative, so this clamp forces `x`
negative and the tooltip renders partly off-screen to the left — which is
exactly the long-category-name scenario this feature is meant to handle.
Should the upper bound also floor at 0 (e.g. `Math.max(size.viewSize[0] -
tooltipWidth, 0)`), or should the tooltip content itself be constrained to the
viewport width?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -861,6 +923,7 @@ export function getLegendProps(
overflow: 'truncate',
width: getLegendWidth(padding.left),
};
+ legend.tooltip = makeLegendTooltip(getLegendWidth(padding.left));
Review Comment:
Left/Right legends now get this hover tooltip unconditionally whenever
`padding` is passed (here and the matching Right case on line 937), unlike
Top/Bottom which only gets it when the new opt-in `horizontalLegendWidth`
argument is provided. Timeseries, MixedTimeseries, and Gantt already call
`getLegendProps` with `padding` and a user-selectable Left/Right legend
orientation, so they'll start showing this new hover tooltip on truncated
legend items even though this PR is scoped to Pie.
Should Left/Right require the same explicit opt-in as Top/Bottom rather than
firing automatically whenever `padding` is present?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -853,6 +870,51 @@ 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.min(
+ Math.max(elRect.x + elRect.width / 2 - tooltipWidth / 2, 0),
+ size.viewSize[0] - tooltipWidth,
+ );
+ // 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 }) => {
+ // Suppress tooltip when text fits — approx 7.5px per char at default
font size
+ const approxMaxChars = Math.floor(maxTextWidth / 7.5);
Review Comment:
This tooltip-suppression check estimates label width as `length * 7.5px`
rather than measuring the actual rendered text, so it can be wrong in both
directions: a label made of wide characters (bold glyphs, CJK, etc.) can be
visibly truncated by ECharts yet still get no tooltip here because its
character count stays under the threshold, while a label of narrow characters
that already fits can still trigger the tooltip.
`measureLegendTextWidth(text, theme)` a few lines up in this same file (used
elsewhere for legend sizing) already measures real rendered width via canvas.
Could the tooltip formatter use that instead of the flat per-character estimate?
--
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]
