rusackas commented on code in PR #43189:
URL: https://github.com/apache/superset/pull/43189#discussion_r4129120072
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -94,9 +94,9 @@ function getLegendLabel(item: LegendDataItem): string {
return String(item.name);
}
-function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+export function measureTextWidth(text: string, theme: SupersetTheme): number {
const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
Review Comment:
fontSize now matches on both sides (markLine label sets theme.fontSizeSM,
measureTextWidth assumes it), pinned by a test assertion. Making the function
accept an explicit font/textStyle for future callers is a fair idea but there's
only one caller diverging in size today, so leaving that as an optional
follow-up rather than blocking here.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
Review Comment:
Fair point, a defined assertion before dereferencing would give a clearer
failure message. Not blocking though, leaving it as an optional cleanup rather
than holding up the merge.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
+ // The names are drawn as markLine labels, which `grid.containLabel` ignores.
+ const longCategory = 'A very long category name that would be clipped';
+ const props = new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({
+ ...datum,
+ 'Y Axis': longCategory,
+ })),
+ },
+ ],
+ });
+
+ const result = transformProps(props as EchartsGanttChartProps);
+ const grid = result.echartOptions.grid as { left: number };
+ const categoryMarkLine = (result.echartOptions.series as any[]).find(
+ series => series.markLine?.label?.formatter === '{b}',
+ );
Review Comment:
Fair point, a defined assertion before dereferencing would give a clearer
failure message. Not blocking though, leaving it as an optional cleanup rather
than holding up the merge.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
+ // The names are drawn as markLine labels, which `grid.containLabel` ignores.
+ const longCategory = 'A very long category name that would be clipped';
+ const props = new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({
+ ...datum,
+ 'Y Axis': longCategory,
+ })),
+ },
+ ],
+ });
+
+ const result = transformProps(props as EchartsGanttChartProps);
+ const grid = result.echartOptions.grid as { left: number };
+ const categoryMarkLine = (result.echartOptions.series as any[]).find(
+ series => series.markLine?.label?.formatter === '{b}',
+ );
+
+ const baseline = transformProps(
+ new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({ ...datum, 'Y Axis': 'a'
})),
+ },
+ ],
+ }) as EchartsGanttChartProps,
+ );
+
+ // a longer category reserves more left padding than a short one
+ expect(grid.left).toBeGreaterThan(
+ (baseline.echartOptions.grid as { left: number }).left,
+ );
+ // and the label truncates instead of overflowing whatever was reserved
+ expect(categoryMarkLine.markLine.label.overflow).toBe('truncate');
+ expect(categoryMarkLine.markLine.label.width).toBeGreaterThan(0);
Review Comment:
Fair point, a defined assertion before dereferencing would give a clearer
failure message. Not blocking though, leaving it as an optional cleanup rather
than holding up the merge.
--
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]
