Fix legend issues in pie and XY, add some tests

x-pack/plugins/lens/public/pie_visualization/render_function.test.tsx

@@ -0,0 +1,113 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import React from 'react';
+import { Settings } from '@elastic/charts';
+import { shallow } from 'enzyme';
+import { LensMultiTable } from '../types';
+import { PieComponent } from './render_function';
+import { PieExpressionArgs } from './types';
+
+describe('PieVisualization component', () => {
+  let getFormatSpy: jest.Mock;
+  let convertSpy: jest.Mock;
+
+  beforeEach(() => {
+    convertSpy = jest.fn(x => x);
+    getFormatSpy = jest.fn();
+    getFormatSpy.mockReturnValue({ convert: convertSpy });
+  });
+
+  describe('legend options', () => {
+    const data: LensMultiTable = {
+      type: 'lens_multitable',
+      tables: {
+        first: {
+          type: 'kibana_datatable',
+          columns: [
+            { id: 'a', name: 'a' },
+            { id: 'b', name: 'b' },
+            { id: 'c', name: 'c' },
+          ],
+          rows: [
+            { a: 6, b: 2, c: 'I', d: 'Row 1' },
+            { a: 1, b: 5, c: 'J', d: 'Row 2' },
+          ],
+        },
+      },
+    };
+
+    const args: PieExpressionArgs = {
+      shape: 'pie',
+      groups: ['a', 'b'],
+      metric: 'c',
+      numberDisplay: 'hidden',
+      categoryDisplay: 'default',
+      legendDisplay: 'default',
+      nestedLegend: false,
+      percentDecimals: 3,
+      hideLabels: false,
+    };
+
+    function getDefaultArgs() {
+      return {
+        data,
+        formatFactory: getFormatSpy,
+        isDarkMode: false,
+        chartTheme: {},
+        executeTriggerActions: jest.fn(),
+      };
+    }
+
+    test('it shows legend for 2 groups using default legendDisplay', () => {
+      const component = shallow(<PieComponent args={args} {...getDefaultArgs()} />);
+      expect(component.find(Settings).prop('showLegend')).toEqual(true);
+    });
+
+    test('it hides legend for 1 group using default legendDisplay', () => {
+      const component = shallow(
+        <PieComponent args={{ ...args, groups: ['a'] }} {...getDefaultArgs()} />
+      );
+      expect(component.find(Settings).prop('showLegend')).toEqual(false);
+    });
+
+    test('it hides legend that would show otherwise in preview mode', () => {
+      const component = shallow(
+        <PieComponent args={{ ...args, hideLabels: true }} {...getDefaultArgs()} />
+      );
+      expect(component.find(Settings).prop('showLegend')).toEqual(false);
+    });
+
+    test('it hides legend with 2 groups for treemap', () => {
+      const component = shallow(
+        <PieComponent args={{ ...args, shape: 'treemap' }} {...getDefaultArgs()} />
+      );
+      expect(component.find(Settings).prop('showLegend')).toEqual(false);
+    });
+
+    test('it shows treemap legend only when forced on', () => {
+      const component = shallow(
+        <PieComponent
+          args={{ ...args, legendDisplay: 'show', shape: 'treemap' }}
+          {...getDefaultArgs()}
+        />
+      );
+      expect(component.find(Settings).prop('showLegend')).toEqual(true);
+    });
+
+    test('it defaults to 1-level legend depth', () => {
+      const component = shallow(<PieComponent args={args} {...getDefaultArgs()} />);
+      expect(component.find(Settings).prop('legendMaxDepth')).toEqual(1);
+    });
+
+    test('it shows nested legend only when forced on', () => {
+      const component = shallow(
+        <PieComponent args={{ ...args, nestedLegend: true }} {...getDefaultArgs()} />
+      );
+      expect(component.find(Settings).prop('legendMaxDepth')).toBeUndefined();
+    });
+  });
+});

x-pack/plugins/lens/public/pie_visualization/render_function.tsx

@@ -10,6 +10,7 @@ import { FormattedMessage } from '@kbn/i18n/react';
 import { EuiText } from '@elastic/eui';
 // @ts-ignore no types
 import { euiPaletteColorBlindBehindText } from '@elastic/eui/lib/services';
+import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json';
 import {
   Chart,
   Datum,
@@ -108,14 +109,8 @@ export function PieComponent(
         return String(d);
       },
       fillLabel:
-        shape === 'treemap' && layerIndex < columnGroups.length - 1
-          ? {
-              ...fillLabel,
-              // For the treemap, hide all text except for the most detailed slice
-              // Otherwise text will overlap and be unreadable
-              valueFormatter: () => '',
-              textColor: 'rgba(0,0,0,0)',
-            }
+        isDarkMode && shape === 'treemap'
+          ? { ...fillLabel, textColor: euiDarkVars.euiTextColor }
           : fillLabel,
       shape: {
         fillColor: d => {
@@ -131,6 +126,10 @@ export function PieComponent(
           // Look up round-robin color from default palette
           const outputColor = sortedColors[parentIndex % sortedColors.length];
 
+          if (shape === 'treemap' && layerIndex < columnGroups.length - 1) {
+            return 'rgba(0,0,0,0)';
+          }
+
           if (shape === 'treemap') {
             return outputColor;
           }
@@ -227,7 +226,15 @@ export function PieComponent(
     <VisualizationContainer className="lnsPieExpression__container" isReady={state.isReady}>
       <Chart>
         <Settings
-          showLegend={!hideLabels && (legendDisplay !== 'hide' || columnGroups.length > 1)}
+          // Legend is hidden in many scenarios
+          // - Tiny preview
+          // - Treemap does not need a legend because it uses category labels
+          // - Single layer pie/donut usually shows text, does not need legend
+          showLegend={
+            !hideLabels &&
+            (legendDisplay === 'show' ||
+              (legendDisplay === 'default' && columnGroups.length > 1 && shape !== 'treemap'))
+          }
           legendMaxDepth={nestedLegend ? undefined : 1 /* Color is based only on first layer */}
           onElementClick={args => {
             const context = getFilterContext(

x-pack/plugins/lens/public/pie_visualization/suggestions.ts

@@ -45,8 +45,8 @@ export function suggestions({
     }
 
     results.push({
-      title: getTitle({ table, state, keptLayerIds }),
-      score: 0.6,
+      title: getTitle(newShape, { table, state, keptLayerIds }),
+      score: state && state.shape !== 'treemap' ? 0.6 : 0.4,
       state: {
         shape: newShape,
         layers: [
@@ -78,9 +78,11 @@ export function suggestions({
     results.push({
       title:
         state?.shape === 'treemap'
-          ? getTitle({ table, state, keptLayerIds })
+          ? getTitle('treemap', { table, state, keptLayerIds })
           : i18n.translate('xpack.lens.pie.treemapLabel', { defaultMessage: 'Treemap' }),
-      score: 0.5,
+      // Use a higher score when currently active, to prevent chart type switching
+      // on the user unintentionally
+      score: state?.shape === 'treemap' ? 0.7 : 0.5,
       state: {
         shape: 'treemap',
         layers: [
@@ -111,7 +113,16 @@ export function suggestions({
   return results;
 }
 
-export function getTitle({ table, state }: SuggestionRequest<PieVisualizationState>) {
+export function getTitle(
+  newShape: PieVisualizationState['shape'],
+  { table, state }: SuggestionRequest<PieVisualizationState>
+) {
+  if (state && newShape !== state.shape) {
+    return i18n.translate('xpack.lens.pie.suggestionLabel', {
+      defaultMessage: 'As {chartName}',
+      values: { chartName: state ? CHART_NAMES[newShape].label : CHART_NAMES.donut.label },
+    });
+  }
   return table.changeType === 'unchanged'
     ? i18n.translate('xpack.lens.pie.suggestionLabel', {
         defaultMessage: 'As {chartName}',

x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx

@@ -1258,5 +1258,55 @@ describe('xy_expression', () => {
       // This one series should only have one row, even though 2 are sent
       expect(series.prop('data')).toEqual([{ a: 1, b: 5, c: 'J', d: 'Row 2' }]);
     });
+
+    test('it should show legend for split series, even with one row', () => {
+      const data: LensMultiTable = {
+        type: 'lens_multitable',
+        tables: {
+          first: {
+            type: 'kibana_datatable',
+            columns: [
+              { id: 'a', name: 'a' },
+              { id: 'b', name: 'b' },
+              { id: 'c', name: 'c' },
+            ],
+            rows: [{ a: 1, b: 5, c: 'J' }],
+          },
+        },
+      };
+
+      const args: XYArgs = {
+        xTitle: '',
+        yTitle: '',
+        legend: { type: 'lens_xy_legendConfig', isVisible: true, position: Position.Top },
+        layers: [
+          {
+            layerId: 'first',
+            seriesType: 'line',
+            xAccessor: 'a',
+            accessors: ['c'],
+            splitAccessor: 'b',
+            columnToLabel: '',
+            xScaleType: 'ordinal',
+            yScaleType: 'linear',
+            isHistogram: false,
+          },
+        ],
+      };
+
+      const component = shallow(
+        <XYChart
+          data={data}
+          args={args}
+          formatFactory={getFormatSpy}
+          timeZone="UTC"
+          chartTheme={{}}
+          histogramBarTarget={50}
+          executeTriggerActions={executeTriggerActions}
+        />
+      );
+
+      expect(component.find(Settings).prop('showLegend')).toEqual(true);
+    });
   });
 });

x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx

@@ -225,7 +225,9 @@ export function XYChart({
   }
 
   const chartHasMoreThanOneSeries =
-    filteredLayers.length > 1 || filteredLayers.some(layer => layer.accessors.length > 2);
+    filteredLayers.length > 1 ||
+    filteredLayers.some(layer => layer.accessors.length > 1) ||
+    filteredLayers.some(layer => layer.splitAccessor);
   const shouldRotate = isHorizontalChart(filteredLayers);
 
   const xTitle = (xAxisColumn && xAxisColumn.name) || args.xTitle;