chore(charts): echarts left padding too big and automation of title (#36993)

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts

@@ -242,8 +242,10 @@ export default function transformProps(
     // @ts-ignore
     ...outlierData,
   ];
-  const addYAxisTitleOffset = !!yAxisTitle;
-  const addXAxisTitleOffset = !!xAxisTitle;
+  const addYAxisTitleOffset =
+    !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0;
+  const addXAxisTitleOffset =
+    !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
   const chartPadding = getPadding(
     true,
     legendOrientation,

superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts

@@ -576,8 +576,11 @@ export default function transformProps(
       ? getXAxisFormatter(xAxisTimeFormat)
       : String;
 
-  const addYAxisTitleOffset = !!(yAxisTitle || yAxisTitleSecondary);
-  const addXAxisTitleOffset = !!xAxisTitle;
+  const addYAxisTitleOffset =
+    !!(yAxisTitle || yAxisTitleSecondary) &&
+    convertInteger(yAxisTitleMargin) !== 0;
+  const addXAxisTitleOffset =
+    !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
 
   const chartPadding = getPadding(
     showLegend,

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

@@ -574,8 +574,10 @@ export default function transformProps(
     onLegendScroll,
   } = hooks;
 
-  const addYAxisLabelOffset = !!yAxisTitle;
-  const addXAxisLabelOffset = !!xAxisTitle;
+  const addYAxisLabelOffset =
+    !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0;
+  const addXAxisLabelOffset =
+    !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
   const padding = getPadding(
     showLegend,
     legendOrientation,

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts

@@ -661,7 +661,9 @@ export function getPadding(
       top:
         yAxisTitlePosition && yAxisTitlePosition === 'Top'
           ? TIMESERIES_CONSTANTS.gridOffsetTop + (Number(yAxisTitleMargin) || 0)
-          : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
+          : yAxisTitlePosition === 'Left'
+            ? TIMESERIES_CONSTANTS.gridOffsetTop
+            : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
       bottom:
         zoomable && !isHorizontal
           ? TIMESERIES_CONSTANTS.gridOffsetBottomZoomable + xAxisOffset

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts

@@ -21,12 +21,16 @@ import { GenericDataType } from '@apache-superset/core/api/core';
 import { supersetTheme } from '@apache-superset/core/ui';
 import type { SeriesOption } from 'echarts';
 import { EchartsTimeseriesSeriesType } from '../../src';
+import { TIMESERIES_CONSTANTS } from '../../src/constants';
+import { LegendOrientation } from '../../src/types';
 import {
   transformSeries,
   transformNegativeLabelsPosition,
+  getPadding,
 } from '../../src/Timeseries/transformers';
 import transformProps from '../../src/Timeseries/transformProps';
 import { EchartsTimeseriesChartProps } from '../../src/types';
+import * as seriesUtils from '../../src/utils/series';
 
 // Mock the colorScale function
 const mockColorScale = jest.fn(
@@ -265,3 +269,167 @@ test('should configure time axis labels to show max label for last month visibil
     }),
   );
 });
+
+function setupGetChartPaddingMock(): jest.SpyInstance {
+  // Mock getChartPadding to return the padding object as-is for easier testing
+  const getChartPaddingSpy = jest.spyOn(seriesUtils, 'getChartPadding');
+  getChartPaddingSpy.mockImplementation(
+    (
+      show: boolean,
+      orientation: LegendOrientation,
+      margin: string | number | null | undefined,
+      padding:
+        | {
+            bottom?: number;
+            left?: number;
+            right?: number;
+            top?: number;
+          }
+        | undefined,
+    ) => {
+      return {
+        bottom: padding?.bottom ?? 0,
+        left: padding?.left ?? 0,
+        right: padding?.right ?? 0,
+        top: padding?.top ?? 0,
+      };
+    },
+  );
+  return getChartPaddingSpy;
+}
+
+test('getPadding should only affect left margin when Y axis title position is Left', () => {
+  const getChartPaddingSpy = setupGetChartPaddingMock();
+  try {
+    const result = getPadding(
+      false, // showLegend
+      LegendOrientation.Top, // legendOrientation
+      true, // addYAxisTitleOffset
+      false, // zoomable
+      null, // margin
+      false, // addXAxisTitleOffset
+      'Left', // yAxisTitlePosition
+      30, // yAxisTitleMargin
+      0, // xAxisTitleMargin
+      false, // isHorizontal
+    );
+
+    // Top should be base value, not affected by Left position
+    expect(result.top).toBe(TIMESERIES_CONSTANTS.gridOffsetTop);
+    // Left should include the margin
+    expect(result.left).toBe(TIMESERIES_CONSTANTS.gridOffsetLeft + 30);
+    // Bottom should be base value
+    expect(result.bottom).toBe(TIMESERIES_CONSTANTS.gridOffsetBottom);
+    // Right should be base value
+    expect(result.right).toBe(TIMESERIES_CONSTANTS.gridOffsetRight);
+  } finally {
+    getChartPaddingSpy.mockRestore();
+  }
+});
+
+test('getPadding should only affect top margin when Y axis title position is Top', () => {
+  const getChartPaddingSpy = setupGetChartPaddingMock();
+  try {
+    const result = getPadding(
+      false, // showLegend
+      LegendOrientation.Top, // legendOrientation
+      true, // addYAxisTitleOffset
+      false, // zoomable
+      null, // margin
+      false, // addXAxisTitleOffset
+      'Top', // yAxisTitlePosition
+      30, // yAxisTitleMargin
+      0, // xAxisTitleMargin
+      false, // isHorizontal
+    );
+
+    // Top should include the margin
+    expect(result.top).toBe(TIMESERIES_CONSTANTS.gridOffsetTop + 30);
+    // Left should be base value, not affected by Top position
+    expect(result.left).toBe(TIMESERIES_CONSTANTS.gridOffsetLeft);
+    // Bottom should be base value
+    expect(result.bottom).toBe(TIMESERIES_CONSTANTS.gridOffsetBottom);
+    // Right should be base value
+    expect(result.right).toBe(TIMESERIES_CONSTANTS.gridOffsetRight);
+  } finally {
+    getChartPaddingSpy.mockRestore();
+  }
+});
+
+test('getPadding should use yAxisOffset for top when position is not specified and addYAxisTitleOffset is true', () => {
+  const getChartPaddingSpy = setupGetChartPaddingMock();
+  try {
+    const result = getPadding(
+      false, // showLegend
+      LegendOrientation.Top, // legendOrientation
+      true, // addYAxisTitleOffset
+      false, // zoomable
+      null, // margin
+      false, // addXAxisTitleOffset
+      undefined, // yAxisTitlePosition (not specified)
+      0, // yAxisTitleMargin
+      0, // xAxisTitleMargin
+      false, // isHorizontal
+    );
+
+    // Top should include yAxisOffset
+    expect(result.top).toBe(
+      TIMESERIES_CONSTANTS.gridOffsetTop +
+        TIMESERIES_CONSTANTS.yAxisLabelTopOffset,
+    );
+    // Left should be base value
+    expect(result.left).toBe(TIMESERIES_CONSTANTS.gridOffsetLeft);
+  } finally {
+    getChartPaddingSpy.mockRestore();
+  }
+});
+
+test('getPadding should not add yAxisOffset when addYAxisTitleOffset is false', () => {
+  const getChartPaddingSpy = setupGetChartPaddingMock();
+  try {
+    const result = getPadding(
+      false, // showLegend
+      LegendOrientation.Top, // legendOrientation
+      false, // addYAxisTitleOffset
+      false, // zoomable
+      null, // margin
+      false, // addXAxisTitleOffset
+      undefined, // yAxisTitlePosition
+      0, // yAxisTitleMargin
+      0, // xAxisTitleMargin
+      false, // isHorizontal
+    );
+
+    // Top should be base value only
+    expect(result.top).toBe(TIMESERIES_CONSTANTS.gridOffsetTop);
+    // Left should be base value
+    expect(result.left).toBe(TIMESERIES_CONSTANTS.gridOffsetLeft);
+  } finally {
+    getChartPaddingSpy.mockRestore();
+  }
+});
+
+test('getPadding should handle Left position with zero margin correctly', () => {
+  const getChartPaddingSpy = setupGetChartPaddingMock();
+  try {
+    const result = getPadding(
+      false, // showLegend
+      LegendOrientation.Top, // legendOrientation
+      true, // addYAxisTitleOffset
+      false, // zoomable
+      null, // margin
+      false, // addXAxisTitleOffset
+      'Left', // yAxisTitlePosition
+      0, // yAxisTitleMargin (zero)
+      0, // xAxisTitleMargin
+      false, // isHorizontal
+    );
+
+    // Top should be base value, not affected
+    expect(result.top).toBe(TIMESERIES_CONSTANTS.gridOffsetTop);
+    // Left should be base value only (margin is 0)
+    expect(result.left).toBe(TIMESERIES_CONSTANTS.gridOffsetLeft);
+  } finally {
+    getChartPaddingSpy.mockRestore();
+  }
+});