Re: [PR] fix(treemap): restore sequential coloring + remove label box, 1px gap (fixes #36807) [superset]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #36847:
URL: https://github.com/apache/superset/pull/36847#discussion_r2673599581


##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
     },
     series,
   };
+
+  // this will add visualMap for sequential coloring when appropriate
+  if (useSequential) {
+    const startColor = colorFn('min', sliceId);
+    const endColor = colorFn('max', sliceId);
+    // try to pick a proper sequential palette from registry using the current colorScheme as a hint
+    const seqScheme = sequentialRegistry?.get?.(
+      (formData as any)?.linearColorScheme || (colorScheme as string),
+    );
+    const inRangeColors: string[] = (
+      seqScheme?.colors && seqScheme.colors.length
+        ? seqScheme.colors
+        : [startColor, endColor]
+    ).filter((c): c is string => !!c);

Review Comment:
   Using a categorical color function with 'min' and 'max' as category keys may not produce appropriate colors for a sequential gradient. If the sequential scheme is not found in the registry, consider using predefined sequential color palettes or theme-based sequential colors instead of categorical colors.
   ```suggestion
       // try to pick a proper sequential palette from registry using the current colorScheme as a hint
       const seqScheme = sequentialRegistry?.get?.(
         (formData as any)?.linearColorScheme || (colorScheme as string),
       );
       let inRangeColors: string[] = [];
   
       if (seqScheme?.colors && seqScheme.colors.length) {
         inRangeColors = seqScheme.colors.filter((c): c is string => !!c);
       } else if (theme) {
         // Fall back to theme-based sequential colors instead of categorical colors
         const primary = (theme as any).colorPrimary ?? (theme as any).colors?.[0];
         const secondary = (theme as any).colors?.[1] ?? primary;
         if (primary) {
           inRangeColors = secondary ? [primary, secondary] : [primary];
         }
       }
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -205,8 +221,7 @@ export default function transformProps(
           item = {
             ...item,
             itemStyle: {
-              colorAlpha: OpacityEnum.SemiTransparent,
-              color: theme.colorText,
+              opacity: OpacityEnum.SemiTransparent,
               borderColor: theme.colorBgBase,
               borderWidth: 2,
             },

Review Comment:
   The itemStyle object is completely replaced when filtering, which discards the previous properties including gapWidth and color (for categorical mode). Consider spreading the existing itemStyle properties to preserve them: `itemStyle: { ...item.itemStyle, opacity: OpacityEnum.SemiTransparent, borderColor: theme.colorBgBase, borderWidth: 2 }`



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
     },
     series,
   };
+
+  // this will add visualMap for sequential coloring when appropriate

Review Comment:
   The comment says "this will add visualMap for sequential coloring when appropriate" but would be more accurate as "Add visualMap configuration for sequential coloring when metric has varying values".
   ```suggestion
     // Add visualMap configuration for sequential coloring when metric has varying values
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
   const metricLabel = getMetricLabel(metric);
   const groupbyLabels = groupby.map(getColumnLabel);
   const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+  // this will actually prepare sequential color mapping when metric values are numeric

Review Comment:
   The comment says "this will actually prepare sequential color mapping" but the code only extracts and validates metric values for determining whether to use sequential coloring. Consider updating the comment to be more precise, such as "Extract and validate metric values to determine if sequential coloring should be applied".
   ```suggestion
     // Extract and validate metric values to determine if sequential coloring should be applied
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/test/Treemap/transformProps.test.ts:
##########
@@ -74,4 +74,39 @@ describe('Treemap transformProps', () => {
       }),
     );
   });
+
+  it('auto mode: should add visualMap for numeric varying metric', () => {
+    const result = transformProps(chartProps as EchartsTreemapChartProps);
+    expect(result.echartOptions).toHaveProperty('visualMap');
+    // @ts-ignore
+    expect(result.echartOptions.visualMap.min).toBeCloseTo(2.5);
+    // @ts-ignore
+    expect(result.echartOptions.visualMap.max).toBeCloseTo(10);

Review Comment:
   The use of @ts-ignore comments suppresses TypeScript type checking. Instead, properly type the visualMap property or use optional chaining with type assertions to maintain type safety while accessing these properties.
   ```suggestion
       const visualMap = (result.echartOptions as { visualMap: { min: number; max: number } }).visualMap;
       expect(visualMap.min).toBeCloseTo(2.5);
       expect(visualMap.max).toBeCloseTo(10);
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
     },
     series,
   };
+
+  // this will add visualMap for sequential coloring when appropriate
+  if (useSequential) {
+    const startColor = colorFn('min', sliceId);
+    const endColor = colorFn('max', sliceId);
+    // try to pick a proper sequential palette from registry using the current colorScheme as a hint
+    const seqScheme = sequentialRegistry?.get?.(
+      (formData as any)?.linearColorScheme || (colorScheme as string),

Review Comment:
   Using type assertion `formData as any` bypasses type checking. Consider extending the EchartsTreemapFormData type definition to include linearColorScheme if it's a valid property, or check if the property exists in a type-safe manner.



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
   const metricLabel = getMetricLabel(metric);
   const groupbyLabels = groupby.map(getColumnLabel);
   const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+  // this will actually prepare sequential color mapping when metric values are numeric
+  const metricValues = (data || [])
+    .map(row => {
+      const v = row[metricLabel as string];
+      return typeof v === 'number' ? v : Number(v);
+    })
+    .filter(v => Number.isFinite(v));
+  const minMetricValue = metricValues.length ? Math.min(...metricValues) : 0;
+  const maxMetricValue = metricValues.length ? Math.max(...metricValues) : 0;

Review Comment:
   Using the spread operator with Math.min and Math.max on potentially large arrays could cause a stack overflow error if metricValues is very large. Consider using a reduce-based approach or iterating through the array to find min/max values for better performance and safety with large datasets.
   ```suggestion
     const { minMetricValue, maxMetricValue } = metricValues.length
       ? metricValues.reduce(
           (acc, v) => ({
             minMetricValue: v < acc.minMetricValue ? v : acc.minMetricValue,
             maxMetricValue: v > acc.maxMetricValue ? v : acc.maxMetricValue,
           }),
           { minMetricValue: metricValues[0], maxMetricValue: metricValues[0] },
         )
       : { minMetricValue: 0, maxMetricValue: 0 };
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
     },
     series,
   };
+
+  // this will add visualMap for sequential coloring when appropriate
+  if (useSequential) {
+    const startColor = colorFn('min', sliceId);
+    const endColor = colorFn('max', sliceId);
+    // try to pick a proper sequential palette from registry using the current colorScheme as a hint
+    const seqScheme = sequentialRegistry?.get?.(
+      (formData as any)?.linearColorScheme || (colorScheme as string),
+    );
+    const inRangeColors: string[] = (
+      seqScheme?.colors && seqScheme.colors.length
+        ? seqScheme.colors
+        : [startColor, endColor]
+    ).filter((c): c is string => !!c);
+    if (inRangeColors.length === 0) {
+      // Fall back to a safe theme color if no valid sequential colors are available
+      inRangeColors.push(theme?.colorPrimary ?? '#000000');
+    }
+    // assign visualMap in a type-safe way
+    (echartOptions as any).visualMap = {

Review Comment:
   The comment states "assign visualMap in a type-safe way" but the implementation uses type assertion with "as any". Consider defining a proper type that includes the visualMap property or using a more type-safe approach to extend the echartOptions object.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org