sadpandajoe commented on code in PR #44147:
URL: https://github.com/apache/superset/pull/44147#discussion_r4172562247
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -425,6 +428,44 @@ export default function transformProps(
{},
);
+ const wrapTextByPixels = (text: string, maxWidth: number): string => {
+ const words = text.split(' ');
+ const lines: string[] = [];
+ let currentLine = '';
+ for (const word of words) {
+ const testLine = currentLine ? `${currentLine} ${word}` : word;
+ if (measureTextWidth(testLine, theme) <= maxWidth) {
+ currentLine = testLine;
+ } else {
+ if (currentLine) {
+ lines.push(currentLine);
+ }
+ currentLine = word;
+ }
+ }
+ if (currentLine) {
+ lines.push(currentLine);
+ }
+ return lines.join('\n');
+ };
+
+ const truncateTextByPixels = (text: string, maxWidth: number): string => {
+ if (measureTextWidth(text, theme) <= maxWidth) {
+ return text;
+ }
+ let left = 0;
+ let right = text.length;
+ while (left < right) {
+ const mid = Math.floor((left + right) / 2);
+ if (measureTextWidth(`${text.slice(0, mid)}...`, theme) <= maxWidth) {
+ left = mid + 1;
+ } else {
+ right = mid;
+ }
+ }
+ return `${text.slice(0, left - 1)}...`;
Review Comment:
When the positive width is smaller than `...` (for example, 1px), no
candidate fits and `left` stays zero, so `slice(0, -1)` returns almost the
entire original label before appending the ellipsis. Could this handle an
unfittable marker without a negative slice, and cover that boundary in the
formatter test?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -425,6 +428,44 @@ export default function transformProps(
{},
);
+ const wrapTextByPixels = (text: string, maxWidth: number): string => {
+ const words = text.split(' ');
+ const lines: string[] = [];
+ let currentLine = '';
+ for (const word of words) {
+ const testLine = currentLine ? `${currentLine} ${word}` : word;
+ if (measureTextWidth(testLine, theme) <= maxWidth) {
+ currentLine = testLine;
+ } else {
+ if (currentLine) {
+ lines.push(currentLine);
+ }
+ currentLine = word;
Review Comment:
With Overflow set to Break, a long identifier or CJK category without spaces
is returned unchanged because an oversized word is never split, so inside
labels can still overlap adjacent slices despite the configured maximum width.
Could oversized tokens be wrapped too, with a formatter test asserting that
every output line fits the limit?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -425,6 +428,44 @@ export default function transformProps(
{},
);
+ const wrapTextByPixels = (text: string, maxWidth: number): string => {
+ const words = text.split(' ');
+ const lines: string[] = [];
+ let currentLine = '';
+ for (const word of words) {
+ const testLine = currentLine ? `${currentLine} ${word}` : word;
+ if (measureTextWidth(testLine, theme) <= maxWidth) {
+ currentLine = testLine;
+ } else {
+ if (currentLine) {
+ lines.push(currentLine);
+ }
+ currentLine = word;
+ }
+ }
+ if (currentLine) {
+ lines.push(currentLine);
+ }
+ return lines.join('\n');
+ };
+
+ const truncateTextByPixels = (text: string, maxWidth: number): string => {
+ if (measureTextWidth(text, theme) <= maxWidth) {
+ return text;
+ }
+ let left = 0;
+ let right = text.length;
+ while (left < right) {
+ const mid = Math.floor((left + right) / 2);
+ if (measureTextWidth(`${text.slice(0, mid)}...`, theme) <= maxWidth) {
Review Comment:
For a supported multiline template such as `{name}\n{percent}`, truncating
the whole string can remove the newline and percentage entirely when the name
is long, even though the percentage fits on its own line. Could truncation
apply independently to each line and preserve the template’s line structure?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -600,8 +610,10 @@ export default function transformProps(
legendOrientation,
showLegend,
theme,
- false,
+ false, // zoomable — Pie charts do not use the zoom control
legendState,
+ undefined, // padding — Pie passes width instead
+ Math.min(width, 250), // horizontalLegendWidth: cap at 250px so long
names don't consume the entire row
Review Comment:
Agreed—the 50px deduction still omits the All/Inverse selectors and
pagination controls: on a 300px scrolling legend, a name narrower than the
250px text limit can be clipped by the smaller scroll viewport while the
formatter suppresses its recovery tooltip. Could the text budget and tooltip
decision account for that viewport too?
--
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]
