⚗️ Integrating various feedback from review

dej611 · dej611 · commit b6873789623e · 2020-08-21T19:20:51.000+02:00
diff --git a/x-pack/plugins/lens/public/xy_visualization/index.ts b/x-pack/plugins/lens/public/xy_visualization/index.ts
@@ -10,7 +10,14 @@ import { ExpressionsSetup } from '../../../../../src/plugins/expressions/public'
 import { UI_SETTINGS } from '../../../../../src/plugins/data/public';
 import { xyVisualization } from './xy_visualization';
 import { xyChart, getXyChartRenderer } from './xy_expression';
-import { legendConfig, layerConfig, yAxisConfig, tickLabelsConfig, gridlinesConfig } from './types';
+import {
+  legendConfig,
+  layerConfig,
+  yAxisConfig,
+  tickLabelsConfig,
+  gridlinesConfig,
+  valueLabelsConfig,
+} from './types';
 import { EditorFrameSetup, FormatFactory } from '../types';
 import { ChartsPluginSetup } from '../../../../../src/plugins/charts/public';
 
@@ -41,6 +48,7 @@ export class XyVisualization {
     expressions.registerFunction(() => yAxisConfig);
     expressions.registerFunction(() => tickLabelsConfig);
     expressions.registerFunction(() => gridlinesConfig);
+    expressions.registerFunction(() => valueLabelsConfig);
     expressions.registerFunction(() => layerConfig);
     expressions.registerFunction(() => xyChart);
 
diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts
@@ -146,6 +146,22 @@ export const buildExpression = (
               ],
             },
           ],
+          displayValues: [
+            {
+              type: 'expression',
+              chain: [
+                {
+                  type: 'function',
+                  function: 'lens_xy_displayValuesConfig',
+                  arguments: {
+                    showLabels: [state?.displayValues?.showLabels ?? false],
+                    fontSize: [state?.displayValues?.fontSize ?? 10],
+                    position: [state?.displayValues?.position ?? 'inside'],
+                  },
+                },
+              ],
+            },
+          ],
           layers: validLayers.map((layer) => {
             const columnToLabel: Record<string, string> = {};
 
diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts
@@ -169,6 +169,43 @@ const axisConfig: { [key in keyof AxisConfig]: ArgumentType<AxisConfig[key]> } =
   },
 };
 
+type ValueLabelsConfigResult = ValueLabelConfig & {
+  type: 'lens_xy_displayValuesConfig';
+};
+
+export const valueLabelsConfig: ExpressionFunctionDefinition<
+  'lens_xy_displayValuesConfig',
+  null,
+  ValueLabelConfig,
+  ValueLabelsConfigResult
+> = {
+  name: 'lens_xy_displayValuesConfig',
+  aliases: [],
+  type: 'lens_xy_displayValuesConfig',
+  help: ``,
+  inputTypes: ['null'],
+  args: {
+    showLabels: {
+      types: ['boolean'],
+      help: '',
+    },
+    fontSize: {
+      types: ['number'],
+      help: '',
+    },
+    position: {
+      types: ['string'],
+      help: '',
+    },
+  },
+  fn: function fn(input: unknown, args: ValueLabelConfig) {
+    return {
+      type: 'lens_xy_displayValuesConfig',
+      ...args,
+    };
+  },
+};
+
 type YConfigResult = YConfig & { type: 'lens_xy_yConfig' };
 
 export const yAxisConfig: ExpressionFunctionDefinition<
@@ -199,7 +236,7 @@ export const yAxisConfig: ExpressionFunctionDefinition<
     showValueLabels: {
       types: ['boolean'],
       default: false,
-      help: 'Whether to show value labels on the chart bars (horizontal only)',
+      help: '',
     },
   },
   fn: function fn(input: unknown, args: YConfig) {
@@ -293,6 +330,12 @@ export type SeriesType =
 
 export type YAxisMode = 'auto' | 'left' | 'right';
 
+export interface ValueLabelConfig {
+  showLabels: boolean;
+  fontSize?: number;
+  position?: 'inside' | 'outside';
+}
+
 export interface YConfig {
   forAccessor: string;
   axisMode?: YAxisMode;
@@ -328,6 +371,7 @@ export interface XYArgs {
   showYAxisTitle?: boolean;
   tickLabelsVisibilitySettings?: AxesSettingsConfig & { type: 'lens_xy_tickLabelsConfig' };
   gridlinesVisibilitySettings?: AxesSettingsConfig & { type: 'lens_xy_gridlinesConfig' };
+  displayValues: ValueLabelConfig & { type: 'lens_xy_displayValuesConfig' };
 }
 
 // Persisted parts of the state
@@ -342,6 +386,7 @@ export interface XYState {
   showYAxisTitle?: boolean;
   tickLabelsVisibilitySettings?: AxesSettingsConfig;
   gridlinesVisibilitySettings?: AxesSettingsConfig;
+  displayValues?: ValueLabelConfig;
 }
 
 export type State = XYState;
diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx
@@ -29,6 +29,7 @@ import {
   EuiHorizontalRule,
   EuiTitle,
 } from '@elastic/eui';
+import { EuiFieldNumber } from '@elastic/eui';
 import {
   VisualizationLayerWidgetProps,
   VisualizationDimensionEditorProps,
@@ -83,6 +84,27 @@ const legendOptions: Array<{ id: string; value: 'auto' | 'show' | 'hide'; label:
   },
 ];
 
+const valueLabelsPositioningOptions: Array<{
+  id: string;
+  value: 'inside' | 'outside';
+  label: string;
+}> = [
+  {
+    id: 'xy_valueLabels_inside',
+    value: 'inside',
+    label: i18n.translate('xpack.lens.xyChart.valueLabels.labelsPositioning.inside', {
+      defaultMessage: 'inside',
+    }),
+  },
+  {
+    id: 'xy_valueLabels_outside',
+    value: 'outside',
+    label: i18n.translate('xpack.lens.xyChart.valueLabels.labelsPositioning.outside', {
+      defaultMessage: 'outside',
+    }),
+  },
+];
+
 export function LayerContextMenu(props: VisualizationLayerWidgetProps<State>) {
   const { state, layerId } = props;
   const horizontalOnly = isHorizontalChart(state.layers);
@@ -235,6 +257,20 @@ export function XyToolbar(props: VisualizationToolbarProps<State>) {
     });
   };
 
+  const onValueDisplayVisibilitySettingsChange = (enabled: boolean): void => {
+    setState({
+      ...state,
+      displayValues: {
+        ...state.displayValues,
+        showLabels: enabled,
+      },
+    });
+  };
+
+  const shouldEnableValueLabels = state?.layers.some((layer) => layer.seriesType.includes('bar'));
+  const showValueLabels = Boolean(shouldEnableValueLabels && state?.displayValues?.showLabels);
+  const valueLabelsPositioning = state?.displayValues?.position || 'inside';
+
   const legendMode =
     state?.legend.isVisible && !state?.legend.showSingleSeries
       ? 'auto'
@@ -365,6 +401,80 @@ export function XyToolbar(props: VisualizationToolbarProps<State>) {
             />
           </EuiFormRow>
           <EuiHorizontalRule margin="s" />
+          <EuiFormRow
+            display="columnCompressedSwitch"
+            fullWidth
+            label={i18n.translate('xpack.lens.xyChart.valueLabels.showlabels.label', {
+              defaultMessage: 'Value Labels Display',
+            })}
+          >
+            <EuiSwitch
+              data-test-subj="lnsshowShowValueLabelsSwitch"
+              showLabel={false}
+              label={i18n.translate('xpack.lens.xyChart.valueLabels.showlabels', {
+                defaultMessage: 'Value Labels Display',
+              })}
+              onChange={({ target }) => onValueDisplayVisibilitySettingsChange(target.checked)}
+              checked={showValueLabels}
+              disabled={!shouldEnableValueLabels}
+            />
+          </EuiFormRow>
+          <EuiFormRow
+            display="columnCompressed"
+            fullWidth
+            label={i18n.translate('xpack.lens.xyChart.valueLabels.labelsFontSize.label', {
+              defaultMessage: 'Value Labels size',
+            })}
+          >
+            <EuiFieldNumber
+              value={state?.displayValues?.fontSize || 10}
+              onChange={({ target }) =>
+                setState({
+                  ...state,
+                  displayValues: {
+                    ...state.displayValues,
+                    showLabels: true,
+                    fontSize: Number(target.value),
+                  },
+                })
+              }
+              disabled={!shouldEnableValueLabels}
+            />
+          </EuiFormRow>
+          <EuiFormRow
+            display="columnCompressed"
+            label={i18n.translate('xpack.lens.xyChart.valueLabels.labelsPositioning.label', {
+              defaultMessage: 'Value labels position',
+            })}
+          >
+            <EuiButtonGroup
+              isFullWidth
+              legend={i18n.translate('xpack.lens.xyChart.legendVisibilityLabel', {
+                defaultMessage: 'Value labels position',
+              })}
+              name="valueLabelsPositioning"
+              buttonSize="compressed"
+              options={valueLabelsPositioningOptions}
+              idSelected={
+                valueLabelsPositioningOptions.find(({ value }) => value === valueLabelsPositioning)!
+                  .id
+              }
+              isDisabled={!shouldEnableValueLabels || !showValueLabels}
+              onChange={(optionId) => {
+                const newMode = valueLabelsPositioningOptions.find(({ id }) => id === optionId)!
+                  .value;
+                setState({
+                  ...state,
+                  displayValues: {
+                    ...state.displayValues,
+                    showLabels: true,
+                    position: newMode,
+                  },
+                });
+              }}
+            />
+          </EuiFormRow>
+          <EuiHorizontalRule margin="s" />
           <EuiFormRow
             display="columnCompressed"
             label={i18n.translate('xpack.lens.xyChart.TickLabels', {
@@ -488,11 +598,10 @@ const idPrefix = htmlIdGenerator()();
 
 export function DimensionEditor(props: VisualizationDimensionEditorProps<State>) {
   const { state, setState, layerId, accessor } = props;
-  const horizontalOnly = isHorizontalChart(state.layers);
-  const chartHasMoreThanOneSeries =
-    state.layers.length > 1 ||
-    state.layers.some(({ accessors }) => accessors.length > 1) ||
-    state.layers.some(({ splitAccessor }) => splitAccessor);
+  // const horizontalOnly = isHorizontalChart(state.layers);
+  // const chartHasMoreThanOneSeries =
+  // state.layers.length > 1 || state.layers.some(({ splitAccessor }) => splitAccessor);
+  const shouldEnableValueLabels = state?.layers.some((layer) => layer.seriesType.includes('bar'));
 
   const index = state.layers.findIndex((l) => l.layerId === layerId);
   const layer = state.layers[index];
@@ -501,9 +610,8 @@ export function DimensionEditor(props: VisualizationDimensionEditorProps<State>)
     forAccessor: accessor,
   };
   const axisMode = layerConfig?.axisMode || 'auto';
-  const shouldShowValueLabels = horizontalOnly && !chartHasMoreThanOneSeries;
 
-  const showValueLabels = (shouldShowValueLabels && layerConfig?.showValueLabels) ?? false;
+  const showValueLabels = (shouldEnableValueLabels && layerConfig?.showValueLabels) ?? false;
 
   const createNewYAxisConfigsWithValue = useCallback(
     <K extends keyof YConfig, V extends YConfig[K]>(prop: K, newValue: V) => {
@@ -570,31 +678,30 @@ export function DimensionEditor(props: VisualizationDimensionEditorProps<State>)
           }}
         />
       </EuiFormRow>
-      {shouldShowValueLabels && (
-        <EuiFormRow
-          display="columnCompressedSwitch"
-          fullWidth
+      <EuiFormRow
+        display="columnCompressedSwitch"
+        fullWidth
+        label={i18n.translate('xpack.lens.xyChart.showValueLabels.label', {
+          defaultMessage: 'Show Value Labels',
+        })}
+      >
+        <EuiSwitch
+          data-test-subj="lnsshowShowValueLabelsSwitch"
+          showLabel={false}
           label={i18n.translate('xpack.lens.xyChart.showValueLabels.label', {
             defaultMessage: 'Show Value Labels',
           })}
-        >
-          <EuiSwitch
-            data-test-subj="lnsshowShowValueLabelsSwitch"
-            showLabel={false}
-            label={i18n.translate('xpack.lens.xyChart.showValueLabels.label', {
-              defaultMessage: 'Show Value Labels',
-            })}
-            onChange={({ target }) => {
-              const newYAxisConfigs = createNewYAxisConfigsWithValue(
-                'showValueLabels',
-                target.checked
-              );
-              setState(updateLayer(state, { ...layer, yConfig: newYAxisConfigs }, index));
-            }}
-            checked={showValueLabels}
-          />
-        </EuiFormRow>
-      )}
+          onChange={({ target }) => {
+            const newYAxisConfigs = createNewYAxisConfigsWithValue(
+              'showValueLabels',
+              target.checked
+            );
+            setState(updateLayer(state, { ...layer, yConfig: newYAxisConfigs }, index));
+          }}
+          checked={showValueLabels}
+          disabled={!shouldEnableValueLabels}
+        />
+      </EuiFormRow>
     </EuiForm>
   );
 }
diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx
