From cb9777a0a0d9715cf181b080aa4e9917d297f27c Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 27 Oct 2020 09:26:14 +0100 Subject: [PATCH 01/21] :sparkles: First pass with chart values --- .../xy_visualization/expression.test.tsx | 4 + .../public/xy_visualization/expression.tsx | 62 ++++++++++++- .../lens/public/xy_visualization/index.ts | 2 + .../public/xy_visualization/to_expression.ts | 14 +++ .../lens/public/xy_visualization/types.ts | 35 ++++++++ .../xy_visualization/xy_config_panel.tsx | 87 +++++++++++++++++-- 6 files changed, 196 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx index 9e937399a7969..a7ab152f27c47 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx @@ -246,6 +246,10 @@ const createArgsWithLayers = (layers: LayerArgs[] = [sampleLayer]): XYArgs => ({ isVisible: false, position: Position.Top, }, + valueLabels: { + type: 'lens_xy_valueLabelsConfig', + mode: 'hide' + } axisTitlesVisibilitySettings: { type: 'lens_xy_axisTitlesVisibilityConfig', x: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 4a2c13e1e3520..fa735ca376389 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -125,6 +125,10 @@ export const xyChart: ExpressionFunctionDefinition< defaultMessage: 'Define how missing values are treated', }), }, + valueLabels: { + types: ['lens_xy_valueLabelsConfig'], + help: '', + }, tickLabelsVisibilitySettings: { types: ['lens_xy_tickLabelsConfig'], help: i18n.translate('xpack.lens.xyChart.tickLabelsSettings.help', { @@ -206,6 +210,41 @@ export const getXyChartRenderer = (dependencies: { }, }); +function mergeThemeWithValueLabelsStyling(theme, valuesLabelMode: string, isHorizontal: boolean) { + const VALUE_LABELS_ARE_INSIDE = valuesLabelMode === 'inside'; + const VALUE_LABELS_FONTSIZE = 15; + + if (valuesLabelMode === 'hide') { + return theme; + } + return { + ...theme, + ...{ + chartMargins: { + ...theme.chartMargins, + ...(!VALUE_LABELS_ARE_INSIDE && { + top: isHorizontal ? theme.chartMargins?.top : VALUE_LABELS_FONTSIZE, + right: isHorizontal ? 1.5 * VALUE_LABELS_FONTSIZE : theme.chartMargins?.right, + }), + }, + barSeriesStyle: { + ...theme.barSeriesStyle, + displayValue: { + fontSize: { min: 5, max: VALUE_LABELS_FONTSIZE }, + fill: { textInverted: true, textBorder: true }, + alignment: isHorizontal + ? { + vertical: 'middle', + } + : { horizontal: 'center' }, + offsetX: isHorizontal ? (VALUE_LABELS_ARE_INSIDE ? 5 : -(2 * VALUE_LABELS_FONTSIZE)) : 0, + offsetY: isHorizontal ? 0 : VALUE_LABELS_ARE_INSIDE ? -5 : VALUE_LABELS_FONTSIZE, + }, + }, + }, + }; +} + function getIconForSeriesType(seriesType: SeriesType): IconType { return visualizationTypes.find((c) => c.id === seriesType)!.icon || 'empty'; } @@ -245,7 +284,7 @@ export function XYChart({ onClickValue, onSelectRange, }: XYChartRenderProps) { - const { legend, layers, fittingFunction, gridlinesVisibilitySettings } = args; + const { legend, layers, fittingFunction, gridlinesVisibilitySettings, valueLabels } = args; const chartTheme = chartsThemeService.useChartsTheme(); const chartBaseTheme = chartsThemeService.useChartsBaseTheme(); @@ -387,6 +426,12 @@ export function XYChart({ return style; }; + const baseThemeWithMaybeValueLabels = mergeThemeWithValueLabelsStyling( + chartTheme, + valueLabels?.mode || 'hide', + shouldRotate + ); + return ( safeXAccessorLabelRenderer(d.value), @@ -685,7 +730,18 @@ export function XYChart({ case 'bar_horizontal': case 'bar_horizontal_stacked': case 'bar_horizontal_percentage_stacked': - return ; + const valueLabelsSettings = { + displayValueSettings: { + // TODO: workaround for elastic-charts issue with horizontal bar chart and isValueContainedInElement flag + // eslint-disable-next-line @typescript-eslint/no-explicit-any + // valueFormatter: (d: any): string => associatedAxes?.formatter.convert(d) || '', + showValueLabel: true, + isAlternatingValueLabel: false, + isValueContainedInElement: false, // TODO: check inside mode + hideClippedValue: false, // TODO: check inside mode + }, + }; + return ; case 'area_stacked': case 'area_percentage_stacked': return ( diff --git a/x-pack/plugins/lens/public/xy_visualization/index.ts b/x-pack/plugins/lens/public/xy_visualization/index.ts index 259267236ec49..17ba73241b37f 100644 --- a/x-pack/plugins/lens/public/xy_visualization/index.ts +++ b/x-pack/plugins/lens/public/xy_visualization/index.ts @@ -40,6 +40,7 @@ export class XyVisualization { yAxisConfig, tickLabelsConfig, gridlinesConfig, + valueLabelsConfig, axisTitlesVisibilityConfig, layerConfig, xyChart, @@ -47,6 +48,7 @@ export class XyVisualization { xyVisualization, } = await import('../async_services'); expressions.registerFunction(() => legendConfig); + expressions.registerFunction(() => valueLabelsConfig); expressions.registerFunction(() => yAxisConfig); expressions.registerFunction(() => tickLabelsConfig); expressions.registerFunction(() => gridlinesConfig); diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 5a3c8faa2d476..40d79d6293f89 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -171,6 +171,20 @@ export const buildExpression = ( ], }, ], + valueLabels: [ + { + type: 'expression', + chain: [ + { + type: 'function', + function: 'lens_xy_valueLabelsConfig', + arguments: { + mode: [state?.valueLabels?.mode ?? 'hide'], + }, + }, + ], + }, + ], layers: validLayers.map((layer) => { const columnToLabel: Record = {}; diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts index abee787888787..2382ed785b0a8 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -78,6 +78,35 @@ export const legendConfig: ExpressionFunctionDefinition< }, }; +type ValueLabelsConfigResult = ValueLabelConfig & { + type: 'lens_xy_valueLabelsConfig'; +}; + +export const valueLabelsConfig: ExpressionFunctionDefinition< + 'lens_xy_valueLabelsConfig', + null, + ValueLabelConfig, + ValueLabelsConfigResult +> = { + name: 'lens_xy_valueLabelsConfig', + aliases: [], + type: 'lens_xy_valueLabelsConfig', + help: ``, + inputTypes: ['null'], + args: { + mode: { + types: ['string'], + help: '', + }, + }, + fn: function fn(input: unknown, args: ValueLabelConfig) { + return { + type: 'lens_xy_valueLabelsConfig', + ...args, + }; + }, +}; + export interface AxesSettingsConfig { x: boolean; yLeft: boolean; @@ -358,6 +387,10 @@ export type SeriesType = export type YAxisMode = 'auto' | 'left' | 'right'; +export interface ValueLabelConfig { + mode?: 'hide' | 'inside' | 'outside'; +} + export interface YConfig { forAccessor: string; axisMode?: YAxisMode; @@ -389,6 +422,7 @@ export interface XYArgs { yTitle: string; yRightTitle: string; legend: LegendConfig & { type: 'lens_xy_legendConfig' }; + valueLabels: ValueLabelConfig & { type: 'lens_xy_valueLabelsConfig' }; layers: LayerArgs[]; fittingFunction?: FittingFunction; axisTitlesVisibilitySettings?: AxesSettingsConfig & { @@ -402,6 +436,7 @@ export interface XYArgs { export interface XYState { preferredSeriesType: SeriesType; legend: LegendConfig; + valueLabels: ValueLabelConfig; fittingFunction?: FittingFunction; layers: LayerConfig[]; xTitle?: string; 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 index ee22ee51301df..6857cebb031f0 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -38,6 +38,7 @@ import { getAxesConfiguration } from './axes_configuration'; type UnwrapArray = T extends Array ? P : T; type AxesSettingsConfigKeys = keyof AxesSettingsConfig; +type DisplayValueVisibilityModes = typeof valueLabelsOptions[number]['value']; function updateLayer(state: State, layer: UnwrapArray, index: number): State { const newLayers = [...state.layers]; @@ -73,6 +74,45 @@ const legendOptions: Array<{ id: string; value: 'auto' | 'show' | 'hide'; label: }, ]; +const valueLabelsOptions: Array<{ + id: string; + value: 'hide' | 'inside' | 'outside'; + label: string; +}> = [ + { + id: `value_labels_hide`, + value: 'hide', + label: i18n.translate('xpack.lens.xyChart.valueLabelsVisibility.auto', { + defaultMessage: 'Hide', + }), + }, + { + id: `value_labels_inside`, + value: 'inside', + label: i18n.translate('xpack.lens.xyChart.valueLabelsVisibility.inside', { + defaultMessage: 'Inside', + }), + }, + { + id: `value_labels_outside`, + value: 'outside', + label: i18n.translate('xpack.lens.xyChart.valueLabelsVisibility.hide', { + defaultMessage: 'Outside', + }), + }, +]; + +const valueTooltipContentDisabled = { + stacked: i18n.translate('xpack.lens.xyChart.valuesStackedDisabledHelpText', { + defaultMessage: + 'This setting cannot be applied to stacked bar charts. Try other type of charts', + }), + histogram: i18n.translate('xpack.lens.xyChart.valuesDisabledHelpText', { + defaultMessage: + 'This setting cannot be applied to bar charts having time dimension on the x axis.', + }), +}; + export function LayerContextMenu(props: VisualizationLayerWidgetProps) { const { state, layerId } = props; const horizontalOnly = isHorizontalChart(state.layers); @@ -123,6 +163,17 @@ export function XyToolbar(props: VisualizationToolbarProps) { ['area_stacked', 'area', 'line'].includes(seriesType) ); + const IsBarSingleSeries = state?.layers.some(({ seriesType }) => + ['bar', 'bar_horizontal'].includes(seriesType) + ); + + const isHistogramSeries = + IsBarSingleSeries && + state?.layers.every((layer) => { + console.log({ state, layer }); + return false; + }); + const shouldRotate = state?.layers.length ? isHorizontalChart(state.layers) : false; const axisGroups = getAxesConfiguration(state?.layers, shouldRotate); @@ -190,25 +241,50 @@ export function XyToolbar(props: VisualizationToolbarProps) { : !state?.legend.isVisible ? 'hide' : 'show'; + + const valueLabelsVisibilityMode = state?.valueLabels?.mode || 'hide'; + const tooltipContentValueLabels = !IsBarSingleSeries + ? valueTooltipContentDisabled.stacked + : valueTooltipContentDisabled.histogram; + return ( + + ({ + value, + inputDisplay: label, + }))} + valueOfSelected={valueLabelsVisibilityMode} + onChange={(value: DisplayValueVisibilityModes) => + setState({ ...state, valueLabels: { mode: value } }) + } + itemLayoutAlign="top" + /> + ) { > { return { From db0d21b3265e9d1511cd564cbc642bb680a13d3f Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 27 Oct 2020 14:32:01 +0100 Subject: [PATCH 02/21] :sparkles: Almost completed implementation --- .../xy_visualization/expression.test.tsx | 7 +- .../public/xy_visualization/expression.tsx | 70 +++++++++++-------- .../xy_visualization/xy_config_panel.tsx | 60 ++++++++-------- 3 files changed, 77 insertions(+), 60 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx index a7ab152f27c47..7c82d14b53fa6 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx @@ -248,8 +248,8 @@ const createArgsWithLayers = (layers: LayerArgs[] = [sampleLayer]): XYArgs => ({ }, valueLabels: { type: 'lens_xy_valueLabelsConfig', - mode: 'hide' - } + mode: 'hide', + }, axisTitlesVisibilitySettings: { type: 'lens_xy_axisTitlesVisibilityConfig', x: true, @@ -1798,6 +1798,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, + valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, @@ -1880,6 +1881,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, + valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, @@ -1949,6 +1951,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: true, position: Position.Top }, + valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index fa735ca376389..1de1bc82b93c6 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -20,6 +20,10 @@ import { GeometryValue, XYChartSeriesIdentifier, StackMode, + RecursivePartial, + Theme, + VerticalAlignment, + HorizontalAlignment, } from '@elastic/charts'; import { I18nProvider } from '@kbn/i18n/react'; import { @@ -210,8 +214,11 @@ export const getXyChartRenderer = (dependencies: { }, }); -function mergeThemeWithValueLabelsStyling(theme, valuesLabelMode: string, isHorizontal: boolean) { - const VALUE_LABELS_ARE_INSIDE = valuesLabelMode === 'inside'; +function mergeThemeWithValueLabelsStyling( + theme: RecursivePartial, + valuesLabelMode: string = 'hide', + isHorizontal: boolean +) { const VALUE_LABELS_FONTSIZE = 15; if (valuesLabelMode === 'hide') { @@ -220,13 +227,6 @@ function mergeThemeWithValueLabelsStyling(theme, valuesLabelMode: string, isHori return { ...theme, ...{ - chartMargins: { - ...theme.chartMargins, - ...(!VALUE_LABELS_ARE_INSIDE && { - top: isHorizontal ? theme.chartMargins?.top : VALUE_LABELS_FONTSIZE, - right: isHorizontal ? 1.5 * VALUE_LABELS_FONTSIZE : theme.chartMargins?.right, - }), - }, barSeriesStyle: { ...theme.barSeriesStyle, displayValue: { @@ -234,11 +234,11 @@ function mergeThemeWithValueLabelsStyling(theme, valuesLabelMode: string, isHori fill: { textInverted: true, textBorder: true }, alignment: isHorizontal ? { - vertical: 'middle', + vertical: VerticalAlignment.Middle, } - : { horizontal: 'center' }, - offsetX: isHorizontal ? (VALUE_LABELS_ARE_INSIDE ? 5 : -(2 * VALUE_LABELS_FONTSIZE)) : 0, - offsetY: isHorizontal ? 0 : VALUE_LABELS_ARE_INSIDE ? -5 : VALUE_LABELS_FONTSIZE, + : { horizontal: HorizontalAlignment.Center }, + offsetX: isHorizontal ? 10 : 0, + offsetY: isHorizontal ? 0 : -5, }, }, }, @@ -425,12 +425,23 @@ export function XYChart({ }; return style; }; - - const baseThemeWithMaybeValueLabels = mergeThemeWithValueLabelsStyling( - chartTheme, - valueLabels?.mode || 'hide', - shouldRotate - ); + // Based on the XY toPreviewExpression logic + const isSuggestionPreview = !legend.isVisible && layers.every(({ hide }) => hide); + const shouldShowValueLabels = + // No stacked bar charts + filteredLayers.every((layer) => !layer.seriesType.includes('stacked')) && + // No histogram charts + !isHistogramViz && + // No suggestion charts + !isSuggestionPreview; + + const baseThemeWithMaybeValueLabels = !shouldShowValueLabels + ? chartTheme + : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels?.mode || 'hide', shouldRotate); + + if (!isSuggestionPreview) { + console.log({ shouldShowValueLabels, valueLabels }); + } return ( @@ -646,6 +657,10 @@ export function XYChart({ }); } + const yAxis = yAxesConfiguration.find((axisConfiguration) => + axisConfiguration.series.find((currentSeries) => currentSeries.accessor === accessor) + ); + const seriesProps: SeriesSpec = { splitSeriesAccessors: splitAccessor ? [splitAccessor] : [], stackAccessors: seriesType.includes('stacked') ? [xAccessor as string] : [], @@ -656,9 +671,7 @@ export function XYChart({ xScaleType: xAccessor ? xScaleType : 'ordinal', yScaleType, color: () => getSeriesColor(layer, accessor), - groupId: yAxesConfiguration.find((axisConfiguration) => - axisConfiguration.series.find((currentSeries) => currentSeries.accessor === accessor) - )?.groupId, + groupId: yAxis?.groupId, enableHistogramMode: isHistogram && (seriesType.includes('stacked') || !splitAccessor) && @@ -732,13 +745,14 @@ export function XYChart({ case 'bar_horizontal_percentage_stacked': const valueLabelsSettings = { displayValueSettings: { - // TODO: workaround for elastic-charts issue with horizontal bar chart and isValueContainedInElement flag - // eslint-disable-next-line @typescript-eslint/no-explicit-any - // valueFormatter: (d: any): string => associatedAxes?.formatter.convert(d) || '', - showValueLabel: true, + // This format double fixes two issues in elastic-chart + // * when rotating the chart, the formatter is not correctly picked + // * in some scenarios value labels are not strings, and this breaks the elastic-chart lib + valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', + showValueLabel: shouldShowValueLabels && valueLabels?.mode !== 'hide', isAlternatingValueLabel: false, - isValueContainedInElement: false, // TODO: check inside mode - hideClippedValue: false, // TODO: check inside mode + isValueContainedInElement: true, + hideClippedValue: true, }, }; return ; 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 index 6857cebb031f0..f6edc3bed145d 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -74,6 +74,8 @@ const legendOptions: Array<{ id: string; value: 'auto' | 'show' | 'hide'; label: }, ]; +// TODO: inside/outside logic requires some more work on the elastic-chart side +// as for now limit to a show/hide logic const valueLabelsOptions: Array<{ id: string; value: 'hide' | 'inside' | 'outside'; @@ -90,14 +92,7 @@ const valueLabelsOptions: Array<{ id: `value_labels_inside`, value: 'inside', label: i18n.translate('xpack.lens.xyChart.valueLabelsVisibility.inside', { - defaultMessage: 'Inside', - }), - }, - { - id: `value_labels_outside`, - value: 'outside', - label: i18n.translate('xpack.lens.xyChart.valueLabelsVisibility.hide', { - defaultMessage: 'Outside', + defaultMessage: 'Show', }), }, ]; @@ -163,16 +158,12 @@ export function XyToolbar(props: VisualizationToolbarProps) { ['area_stacked', 'area', 'line'].includes(seriesType) ); - const IsBarSingleSeries = state?.layers.some(({ seriesType }) => + const IsBarNotStacked = state?.layers.some(({ seriesType }) => ['bar', 'bar_horizontal'].includes(seriesType) ); - const isHistogramSeries = - IsBarSingleSeries && - state?.layers.every((layer) => { - console.log({ state, layer }); - return false; - }); + // TODO: Check for histograms to enable/disable value labels visibility + const isHistogramSeries = IsBarNotStacked && false; const shouldRotate = state?.layers.length ? isHorizontalChart(state.layers) : false; const axisGroups = getAxesConfiguration(state?.layers, shouldRotate); @@ -243,9 +234,15 @@ export function XyToolbar(props: VisualizationToolbarProps) { : 'show'; const valueLabelsVisibilityMode = state?.valueLabels?.mode || 'hide'; - const tooltipContentValueLabels = !IsBarSingleSeries + const tooltipContentValueLabels = !IsBarNotStacked ? valueTooltipContentDisabled.stacked : valueTooltipContentDisabled.histogram; + // console.log({ + // legendMode, + // legend: state?.legend, + // valueLabels: state?.valueLabels, + // valueLabelsVisibilityMode, + // }); return ( @@ -253,13 +250,13 @@ export function XyToolbar(props: VisualizationToolbarProps) { ) { defaultMessage: 'Display', })} > - ({ - value, - inputDisplay: label, - }))} - valueOfSelected={valueLabelsVisibilityMode} - onChange={(value: DisplayValueVisibilityModes) => - setState({ ...state, valueLabels: { mode: value } }) + name="valueLabelsDisplay" + buttonSize="compressed" + options={valueLabelsOptions} + idSelected={ + valueLabelsOptions.find(({ value }) => value === valueLabelsVisibilityMode)!.id } - itemLayoutAlign="top" + onChange={(modeId) => { + const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; + setState({ ...state, valueLabels: { mode: newMode } }); + }} /> ) { > { return { From b5c55bb6c01d27ce9cb7fb8128209f0e043f4603 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 28 Oct 2020 18:02:05 +0100 Subject: [PATCH 03/21] :bug: + :white_check_mark: Fix pending checks issues + add tests --- .../__snapshots__/expression.test.tsx.snap | 72 +++++++++++++++++++ .../__snapshots__/to_expression.test.ts.snap | 16 +++++ .../public/xy_visualization/expression.tsx | 4 -- .../xy_visualization/to_expression.test.ts | 31 ++++++++ .../xy_visualization/visualization.test.ts | 4 ++ .../public/xy_visualization/visualization.tsx | 1 + .../xy_visualization/xy_config_panel.test.tsx | 68 +++++++++++++++++- .../xy_visualization/xy_config_panel.tsx | 8 +-- .../xy_visualization/xy_suggestions.test.ts | 12 ++++ .../public/xy_visualization/xy_suggestions.ts | 1 + .../translations/translations/ja-JP.json | 1 - .../translations/translations/zh-CN.json | 1 - 12 files changed, 204 insertions(+), 15 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/expression.test.tsx.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/expression.test.tsx.snap index ca6ca9b2722fd..365bf8f4d6328 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/expression.test.tsx.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/expression.test.tsx.snap @@ -279,6 +279,15 @@ exports[`xy_expression XYChart component it renders bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-a" @@ -334,6 +343,15 @@ exports[`xy_expression XYChart component it renders bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-b" @@ -457,6 +475,15 @@ exports[`xy_expression XYChart component it renders horizontal bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-a" @@ -512,6 +539,15 @@ exports[`xy_expression XYChart component it renders horizontal bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-b" @@ -1019,6 +1055,15 @@ exports[`xy_expression XYChart component it renders stacked bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-a" @@ -1078,6 +1123,15 @@ exports[`xy_expression XYChart component it renders stacked bar 1`] = ` }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-b" @@ -1205,6 +1259,15 @@ exports[`xy_expression XYChart component it renders stacked horizontal bar 1`] = }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-a" @@ -1264,6 +1327,15 @@ exports[`xy_expression XYChart component it renders stacked horizontal bar 1`] = }, ] } + displayValueSettings={ + Object { + "hideClippedValue": true, + "isAlternatingValueLabel": false, + "isValueContainedInElement": true, + "showValueLabel": false, + "valueFormatter": [Function], + } + } enableHistogramMode={false} groupId="left" id="d-b" diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap index b35f915336eee..b20a84df142fc 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap @@ -145,6 +145,22 @@ Object { "title": Array [ "", ], + "valueLabels": Array [ + Object { + "chain": Array [ + Object { + "arguments": Object { + "mode": Array [ + "hide", + ], + }, + "function": "lens_xy_valueLabelsConfig", + "type": "function", + }, + ], + "type": "expression", + }, + ], "xTitle": Array [ "", ], diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 1de1bc82b93c6..8e7092cf236d4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -439,10 +439,6 @@ export function XYChart({ ? chartTheme : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels?.mode || 'hide', shouldRotate); - if (!isSuggestionPreview) { - console.log({ shouldShowValueLabels, valueLabels }); - } - return ( { xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'Carry', tickLabelsVisibilitySettings: { x: false, yLeft: true, yRight: true }, @@ -63,6 +64,7 @@ describe('#toExpression', () => { (xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -83,6 +85,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -109,6 +112,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -132,6 +136,7 @@ describe('#toExpression', () => { xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -152,6 +157,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -187,6 +193,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -213,6 +220,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -234,4 +242,27 @@ describe('#toExpression', () => { yRight: [true], }); }); + + it('should default the valueLabels visibility settings to hide', () => { + const expression = xyVisualization.toExpression( + { + legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: undefined }, + preferredSeriesType: 'bar', + layers: [ + { + layerId: 'first', + seriesType: 'area', + splitAccessor: 'd', + xAccessor: 'a', + accessors: ['b', 'c'], + }, + ], + }, + frame.datasourceLayers + ) as Ast; + expect((expression.chain[0].arguments.valueLabels[0] as Ast).chain[0].arguments).toEqual({ + mode: ['hide'], + }); + }); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts index 3706611575c6b..1c33411deda98 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -14,6 +14,7 @@ import { LensIconChartBar } from '../assets/chart_bar'; function exampleState(): State { return { legend: { position: Position.Bottom, isVisible: true }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -145,6 +146,9 @@ describe('xy_visualization', () => { }, "preferredSeriesType": "bar_stacked", "title": "Empty XY chart", + "valueLabels": Object { + "mode": "hide", + }, } `); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 50fbf3f01e34e..6cfc0cd9d6442 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -140,6 +140,7 @@ export const xyVisualization: Visualization = { state || { title: 'Empty XY chart', legend: { isVisible: true, position: Position.Right }, + valueLabels: { mode: 'hide' }, preferredSeriesType: defaultSeriesType, layers: [ { diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 2114d63fcfacd..76c1330db3e1e 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -21,6 +21,7 @@ describe('XY Config panels', () => { function testState(): State { return { legend: { isVisible: true, position: Position.Right }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -115,7 +116,7 @@ describe('XY Config panels', () => { expect(component.find(EuiSuperSelect).prop('valueOfSelected')).toEqual('Carry'); }); - it('should disable the popover if there is no area or line series', () => { + it('should disable the popover if there is no area, line, bar or bar_horizontal series', () => { const state = testState(); const component = shallow( { setState={jest.fn()} state={{ ...state, - layers: [{ ...state.layers[0], seriesType: 'bar' }], + layers: [{ ...state.layers[0], seriesType: 'bar_stacked' }], fittingFunction: 'Carry', }} /> @@ -132,6 +133,69 @@ describe('XY Config panels', () => { expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(true); }); + it('should show the popover for bar and horizontal_bar series', () => { + const state = testState(); + const component = shallow( + + ); + + expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(false); + }); + + it('should disable the fitting option for bar series', () => { + const state = testState(); + const component = shallow( + + ); + + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsMissingValuesSelect"]') + .prop('disabled') + ).toEqual(true); + }); + + it('should disable the display option for area and line series', () => { + const state = testState(); + const component = shallow( + + ); + + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsValueLabelsDisplay"]') + .prop('isDisabled') + ).toEqual(true); + }); + it('should disable the popover if there is no right axis', () => { const state = testState(); const component = shallow(); 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 index f6edc3bed145d..bef896503b6bd 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -38,7 +38,6 @@ import { getAxesConfiguration } from './axes_configuration'; type UnwrapArray = T extends Array ? P : T; type AxesSettingsConfigKeys = keyof AxesSettingsConfig; -type DisplayValueVisibilityModes = typeof valueLabelsOptions[number]['value']; function updateLayer(state: State, layer: UnwrapArray, index: number): State { const newLayers = [...state.layers]; @@ -237,12 +236,6 @@ export function XyToolbar(props: VisualizationToolbarProps) { const tooltipContentValueLabels = !IsBarNotStacked ? valueTooltipContentDisabled.stacked : valueTooltipContentDisabled.histogram; - // console.log({ - // legendMode, - // legend: state?.legend, - // valueLabels: state?.valueLabels, - // valueLabelsVisibilityMode, - // }); return ( @@ -272,6 +265,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { legend={i18n.translate('xpack.lens.shared.legendVisibilityLabel', { defaultMessage: 'Display', })} + isDisabled={hasNonBarSeries} data-test-subj="lnsValueLabelsDisplay" name="valueLabelsDisplay" buttonSize="compressed" diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts index 1ab00eef0593b..e88d67edf14d4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts @@ -131,6 +131,7 @@ describe('xy_suggestions', () => { keptLayerIds: [], state: { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -243,6 +244,7 @@ describe('xy_suggestions', () => { keptLayerIds: ['first'], state: { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -283,6 +285,7 @@ describe('xy_suggestions', () => { keptLayerIds: ['first', 'second'], state: { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -485,6 +488,7 @@ describe('xy_suggestions', () => { }, state: { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', layers: [ { @@ -537,6 +541,7 @@ describe('xy_suggestions', () => { test('keeps existing seriesType for initial tables', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, fittingFunction: 'None', preferredSeriesType: 'line', layers: [ @@ -570,6 +575,7 @@ describe('xy_suggestions', () => { test('makes a visible seriesType suggestion for unchanged table without split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, gridlinesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -610,6 +616,7 @@ describe('xy_suggestions', () => { test('suggests seriesType and stacking when there is a split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -655,6 +662,7 @@ describe('xy_suggestions', () => { (generateId as jest.Mock).mockReturnValueOnce('dummyCol'); const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, fittingFunction: 'None', preferredSeriesType: 'bar', layers: [ @@ -687,6 +695,7 @@ describe('xy_suggestions', () => { test('suggests stacking for unchanged table that has a split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'None', layers: [ @@ -722,6 +731,7 @@ describe('xy_suggestions', () => { test('keeps column to dimension mappings on extended tables', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -764,6 +774,7 @@ describe('xy_suggestions', () => { test('changes column mappings when suggestion is reorder', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -807,6 +818,7 @@ describe('xy_suggestions', () => { (generateId as jest.Mock).mockReturnValueOnce('dummyCol'); const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, + valueLabels: { mode: 'hide' }, preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts index 47b97af3071ab..e415124e8d516 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts @@ -496,6 +496,7 @@ function buildSuggestion({ const state: State = { legend: currentState ? currentState.legend : { isVisible: true, position: Position.Right }, + valueLabels: currentState?.valueLabels || { mode: 'hide' }, fittingFunction: currentState?.fittingFunction || 'None', xTitle: currentState?.xTitle, yTitle: currentState?.yTitle, diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index 5cabdd62d7c87..96d811c9ce175 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -10965,7 +10965,6 @@ "xpack.lens.xyChart.chartTypeLabel": "チャートタイプ", "xpack.lens.xyChart.chartTypeLegend": "チャートタイプ", "xpack.lens.xyChart.emptyXLabel": "（空）", - "xpack.lens.xyChart.fittingDisabledHelpText": "この設定は折れ線グラフとエリアグラフでのみ適用されます。", "xpack.lens.xyChart.fittingFunction.help": "欠測値の処理方法を定義", "xpack.lens.xyChart.Gridlines": "グリッド線", "xpack.lens.xyChart.gridlinesSettings.help": "xおよびy軸のグリッド線を表示", diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 229938a3c1d08..4c016b382007d 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -10978,7 +10978,6 @@ "xpack.lens.xyChart.chartTypeLabel": "图表类型", "xpack.lens.xyChart.chartTypeLegend": "图表类型", "xpack.lens.xyChart.emptyXLabel": "（空）", - "xpack.lens.xyChart.fittingDisabledHelpText": "此设置仅适用于折线图和面积图。", "xpack.lens.xyChart.fittingFunction.help": "定义处理缺失值的方式", "xpack.lens.xyChart.Gridlines": "网格线", "xpack.lens.xyChart.gridlinesSettings.help": "显示 x 和 y 轴网格线", From 95bfb64ddd725a48d8832ecd978b6895e737d4af Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 28 Oct 2020 19:26:15 +0100 Subject: [PATCH 04/21] :sparkles: Add histogram check + refactor of some shared logic/types --- .../public/xy_visualization/state_helpers.ts | 23 +++++++++++- .../public/xy_visualization/to_expression.ts | 6 +-- .../lens/public/xy_visualization/types.ts | 4 ++ .../xy_visualization/xy_config_panel.tsx | 37 +++++++++++++------ 4 files changed, 52 insertions(+), 18 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts b/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts index 41d18e5199e4c..d485bb52a7bcd 100644 --- a/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts +++ b/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts @@ -5,7 +5,8 @@ */ import { EuiIconType } from '@elastic/eui/src/components/icon/icon'; -import { SeriesType, visualizationTypes, LayerConfig, YConfig } from './types'; +import { FramePublicAPI } from '../types'; +import { SeriesType, visualizationTypes, LayerConfig, YConfig, ValidLayer } from './types'; export function isHorizontalSeries(seriesType: SeriesType) { return ( @@ -37,3 +38,23 @@ export const getSeriesColor = (layer: LayerConfig, accessor: string) => { layer?.yConfig?.find((yConfig: YConfig) => yConfig.forAccessor === accessor)?.color || null ); }; + +export function hasHistogramSeries( + layers: ValidLayer[] = [], + datasourceLayers: FramePublicAPI['datasourceLayers'] +) { + if (!datasourceLayers) { + return false; + } + const validLayers = layers.filter(({ accessors }) => accessors.length); + + return validLayers.some(({ layerId, xAccessor }: ValidLayer) => { + const xAxisOperation = datasourceLayers[layerId].getOperationForColumnId(xAccessor); + return ( + xAxisOperation && + xAxisOperation.isBucketed && + xAxisOperation.scale && + xAxisOperation.scale !== 'ordinal' + ); + }); +} diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 40d79d6293f89..ef1e280bb9640 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -6,13 +6,9 @@ import { Ast } from '@kbn/interpreter/common'; import { ScaleType } from '@elastic/charts'; -import { State, LayerConfig } from './types'; +import { State, ValidLayer } from './types'; import { OperationMetadata, DatasourcePublicAPI } from '../types'; -interface ValidLayer extends LayerConfig { - xAccessor: NonNullable; -} - export const toExpression = ( state: State, datasourceLayers: Record, diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts index 2382ed785b0a8..53798df7c7747 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -407,6 +407,10 @@ export interface LayerConfig { splitAccessor?: string; } +export interface ValidLayer extends LayerConfig { + xAccessor: NonNullable; +} + export type LayerArgs = LayerConfig & { columnToLabel?: string; // Actually a JSON key-value pair yScaleType: 'time' | 'linear' | 'log' | 'sqrt'; 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 index bef896503b6bd..aa7d0ca3f72c3 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -27,8 +27,20 @@ import { VisualizationDimensionEditorProps, VisualizationToolbarProps, } from '../types'; -import { State, SeriesType, visualizationTypes, YAxisMode, AxesSettingsConfig } from './types'; -import { isHorizontalChart, isHorizontalSeries, getSeriesColor } from './state_helpers'; +import { + State, + SeriesType, + visualizationTypes, + YAxisMode, + AxesSettingsConfig, + ValidLayer, +} from './types'; +import { + isHorizontalChart, + isHorizontalSeries, + getSeriesColor, + hasHistogramSeries, +} from './state_helpers'; import { trackUiEvent } from '../lens_ui_telemetry'; import { fittingFunctionDefinitions } from './fitting_functions'; import { ToolbarPopover, LegendSettingsPopover } from '../shared_components'; @@ -151,18 +163,19 @@ export function LayerContextMenu(props: VisualizationLayerWidgetProps) { } export function XyToolbar(props: VisualizationToolbarProps) { - const { state, setState } = props; + const { state, setState, frame } = props; const hasNonBarSeries = state?.layers.some(({ seriesType }) => ['area_stacked', 'area', 'line'].includes(seriesType) ); - const IsBarNotStacked = state?.layers.some(({ seriesType }) => + const hasBarNotStacked = state?.layers.some(({ seriesType }) => ['bar', 'bar_horizontal'].includes(seriesType) ); - // TODO: Check for histograms to enable/disable value labels visibility - const isHistogramSeries = IsBarNotStacked && false; + const isHistogramSeries = Boolean( + hasHistogramSeries(state?.layers as ValidLayer[], frame.datasourceLayers) + ); const shouldRotate = state?.layers.length ? isHorizontalChart(state.layers) : false; const axisGroups = getAxesConfiguration(state?.layers, shouldRotate); @@ -233,9 +246,9 @@ export function XyToolbar(props: VisualizationToolbarProps) { : 'show'; const valueLabelsVisibilityMode = state?.valueLabels?.mode || 'hide'; - const tooltipContentValueLabels = !IsBarNotStacked - ? valueTooltipContentDisabled.stacked - : valueTooltipContentDisabled.histogram; + const tooltipContentValueLabels = isHistogramSeries + ? valueTooltipContentDisabled.histogram + : valueTooltipContentDisabled.stacked; return ( @@ -243,13 +256,13 @@ export function XyToolbar(props: VisualizationToolbarProps) { ) { > { return { From 8bae6e69b21aba9f7def8e91ebe101d0ba121d24 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 10:16:34 +0100 Subject: [PATCH 05/21] :wrench: Increase minimum font size --- x-pack/plugins/lens/public/xy_visualization/expression.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 8e7092cf236d4..289844410fc64 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -230,7 +230,7 @@ function mergeThemeWithValueLabelsStyling( barSeriesStyle: { ...theme.barSeriesStyle, displayValue: { - fontSize: { min: 5, max: VALUE_LABELS_FONTSIZE }, + fontSize: { min: 10, max: VALUE_LABELS_FONTSIZE }, fill: { textInverted: true, textBorder: true }, alignment: isHorizontal ? { From 6f97d07b2403e149ebad74a1d9e79eec9c75a3cd Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 10:40:45 +0100 Subject: [PATCH 06/21] :white_check_mark: Add more tests for value labels corner cases --- .../public/xy_visualization/state_helpers.ts | 2 +- .../xy_visualization/xy_config_panel.test.tsx | 79 ++++++++++++++++++- 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts b/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts index d485bb52a7bcd..bf4ffaa36a870 100644 --- a/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts +++ b/x-pack/plugins/lens/public/xy_visualization/state_helpers.ts @@ -41,7 +41,7 @@ export const getSeriesColor = (layer: LayerConfig, accessor: string) => { export function hasHistogramSeries( layers: ValidLayer[] = [], - datasourceLayers: FramePublicAPI['datasourceLayers'] + datasourceLayers?: FramePublicAPI['datasourceLayers'] ) { if (!datasourceLayers) { return false; diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 76c1330db3e1e..32dc45edb55b4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -116,6 +116,25 @@ describe('XY Config panels', () => { expect(component.find(EuiSuperSelect).prop('valueOfSelected')).toEqual('Carry'); }); + it('should show currently selected value labels display setting', () => { + const state = testState(); + + const component = shallow( + + ); + + expect(component.find(EuiButtonGroup).prop('idSelected')).toEqual('value_labels_inside'); + }); + it('should disable the popover if there is no area, line, bar or bar_horizontal series', () => { const state = testState(); const component = shallow( @@ -125,7 +144,27 @@ describe('XY Config panels', () => { state={{ ...state, layers: [{ ...state.layers[0], seriesType: 'bar_stacked' }], - fittingFunction: 'Carry', + }} + /> + ); + + expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(true); + }); + + it('should disable the popover if there is histogram series', () => { + // make it detect an histogram series + frame.datasourceLayers.first.getOperationForColumnId = jest.fn().mockReturnValueOnce({ + isBucketed: true, + scale: 'interval', + }); + const state = testState(); + const component = shallow( + ); @@ -135,6 +174,7 @@ describe('XY Config panels', () => { it('should show the popover for bar and horizontal_bar series', () => { const state = testState(); + const component = shallow( { ).toEqual(true); }); + it('should keep the display option for bar series with multiple layers', () => { + frame.datasourceLayers = { + ...frame.datasourceLayers, + second: createMockDatasource('test').publicAPIMock, + }; + + const state = testState(); + const component = shallow( + + ); + + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsValueLabelsDisplay"]') + .prop('isDisabled') + ).toEqual(false); + }); + it('should disable the popover if there is no right axis', () => { const state = testState(); const component = shallow(); From 9a1173a76a6f08c06aa39bd8b7081eeab87e60c5 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 12:44:22 +0100 Subject: [PATCH 07/21] :ok_hand: Incorporate review feedback --- .../public/xy_visualization/expression.tsx | 18 +++++++++--------- .../public/xy_visualization/to_expression.ts | 1 + .../xy_visualization/xy_config_panel.tsx | 3 +-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 289844410fc64..227033394b3f9 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -219,7 +219,10 @@ function mergeThemeWithValueLabelsStyling( valuesLabelMode: string = 'hide', isHorizontal: boolean ) { - const VALUE_LABELS_FONTSIZE = 15; + const VALUE_LABELS_MAX_FONTSIZE = 15; + const VALUE_LABELS_MIN_FONTSIZE = 10; + const VALUE_LABELS_VERTICAL_OFFSET = -10; + const VALUE_LABELS_HORIZONTAL_OFFSET = 10; if (valuesLabelMode === 'hide') { return theme; @@ -230,15 +233,15 @@ function mergeThemeWithValueLabelsStyling( barSeriesStyle: { ...theme.barSeriesStyle, displayValue: { - fontSize: { min: 10, max: VALUE_LABELS_FONTSIZE }, + fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE }, fill: { textInverted: true, textBorder: true }, alignment: isHorizontal ? { vertical: VerticalAlignment.Middle, } : { horizontal: HorizontalAlignment.Center }, - offsetX: isHorizontal ? 10 : 0, - offsetY: isHorizontal ? 0 : -5, + offsetX: isHorizontal ? VALUE_LABELS_HORIZONTAL_OFFSET : 0, + offsetY: isHorizontal ? 0 : VALUE_LABELS_VERTICAL_OFFSET, }, }, }, @@ -425,15 +428,12 @@ export function XYChart({ }; return style; }; - // Based on the XY toPreviewExpression logic - const isSuggestionPreview = !legend.isVisible && layers.every(({ hide }) => hide); + const shouldShowValueLabels = // No stacked bar charts filteredLayers.every((layer) => !layer.seriesType.includes('stacked')) && // No histogram charts - !isHistogramViz && - // No suggestion charts - !isSuggestionPreview; + !isHistogramViz; const baseThemeWithMaybeValueLabels = !shouldShowValueLabels ? chartTheme diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index ef1e280bb9640..31873b653fdbe 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -44,6 +44,7 @@ export function toPreviewExpression( ...state.legend, isVisible: false, }, + valueLabels: { mode: 'hide' }, }, datasourceLayers ); 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 index aa7d0ca3f72c3..82cbc24e76872 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -114,8 +114,7 @@ const valueTooltipContentDisabled = { 'This setting cannot be applied to stacked bar charts. Try other type of charts', }), histogram: i18n.translate('xpack.lens.xyChart.valuesDisabledHelpText', { - defaultMessage: - 'This setting cannot be applied to bar charts having time dimension on the x axis.', + defaultMessage: 'This setting cannot be applied to bar charts having time dimension.', }), }; From 75d901376fe2c6f95c6e586326185ecb7dff2080 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 14:46:36 +0100 Subject: [PATCH 08/21] :recycle: Migrate to simpler expression --- .../__snapshots__/to_expression.test.ts.snap | 15 +------- .../xy_visualization/expression.test.tsx | 11 +++--- .../public/xy_visualization/expression.tsx | 7 ++-- .../lens/public/xy_visualization/index.ts | 2 -- .../xy_visualization/to_expression.test.ts | 24 ++++++------- .../public/xy_visualization/to_expression.ts | 17 ++------- .../lens/public/xy_visualization/types.ts | 35 ++----------------- .../xy_visualization/visualization.test.ts | 6 ++-- .../public/xy_visualization/visualization.tsx | 2 +- .../xy_visualization/xy_config_panel.test.tsx | 4 +-- .../xy_visualization/xy_config_panel.tsx | 4 +-- .../xy_visualization/xy_suggestions.test.ts | 24 ++++++------- .../public/xy_visualization/xy_suggestions.ts | 2 +- 13 files changed, 44 insertions(+), 109 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap index b20a84df142fc..982f513ae1019 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/to_expression.test.ts.snap @@ -146,20 +146,7 @@ Object { "", ], "valueLabels": Array [ - Object { - "chain": Array [ - Object { - "arguments": Object { - "mode": Array [ - "hide", - ], - }, - "function": "lens_xy_valueLabelsConfig", - "type": "function", - }, - ], - "type": "expression", - }, + "hide", ], "xTitle": Array [ "", diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx index 7c82d14b53fa6..1de384dd804e4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.test.tsx @@ -246,10 +246,7 @@ const createArgsWithLayers = (layers: LayerArgs[] = [sampleLayer]): XYArgs => ({ isVisible: false, position: Position.Top, }, - valueLabels: { - type: 'lens_xy_valueLabelsConfig', - mode: 'hide', - }, + valueLabels: 'hide', axisTitlesVisibilitySettings: { type: 'lens_xy_axisTitlesVisibilityConfig', x: true, @@ -1798,7 +1795,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, - valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, + valueLabels: 'hide', tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, @@ -1881,7 +1878,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, - valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, + valueLabels: 'hide', tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, @@ -1951,7 +1948,7 @@ describe('xy_expression', () => { yTitle: '', yRightTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: true, position: Position.Top }, - valueLabels: { type: 'lens_xy_valueLabelsConfig', mode: 'hide' }, + valueLabels: 'hide', tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 227033394b3f9..2e7e9318e8f33 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -130,7 +130,8 @@ export const xyChart: ExpressionFunctionDefinition< }), }, valueLabels: { - types: ['lens_xy_valueLabelsConfig'], + types: ['string'], + options: ['hide', 'inside'], help: '', }, tickLabelsVisibilitySettings: { @@ -437,7 +438,7 @@ export function XYChart({ const baseThemeWithMaybeValueLabels = !shouldShowValueLabels ? chartTheme - : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels?.mode || 'hide', shouldRotate); + : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels || 'hide', shouldRotate); return ( @@ -745,7 +746,7 @@ export function XYChart({ // * when rotating the chart, the formatter is not correctly picked // * in some scenarios value labels are not strings, and this breaks the elastic-chart lib valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', - showValueLabel: shouldShowValueLabels && valueLabels?.mode !== 'hide', + showValueLabel: shouldShowValueLabels && valueLabels !== 'hide', isAlternatingValueLabel: false, isValueContainedInElement: true, hideClippedValue: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/index.ts b/x-pack/plugins/lens/public/xy_visualization/index.ts index 17ba73241b37f..259267236ec49 100644 --- a/x-pack/plugins/lens/public/xy_visualization/index.ts +++ b/x-pack/plugins/lens/public/xy_visualization/index.ts @@ -40,7 +40,6 @@ export class XyVisualization { yAxisConfig, tickLabelsConfig, gridlinesConfig, - valueLabelsConfig, axisTitlesVisibilityConfig, layerConfig, xyChart, @@ -48,7 +47,6 @@ export class XyVisualization { xyVisualization, } = await import('../async_services'); expressions.registerFunction(() => legendConfig); - expressions.registerFunction(() => valueLabelsConfig); expressions.registerFunction(() => yAxisConfig); expressions.registerFunction(() => tickLabelsConfig); expressions.registerFunction(() => gridlinesConfig); diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.test.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.test.ts index c752d285619d8..04581593f2773 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.test.ts @@ -39,7 +39,7 @@ describe('#toExpression', () => { xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'Carry', tickLabelsVisibilitySettings: { x: false, yLeft: true, yRight: true }, @@ -64,7 +64,7 @@ describe('#toExpression', () => { (xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -85,7 +85,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -112,7 +112,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -136,7 +136,7 @@ describe('#toExpression', () => { xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -157,7 +157,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -193,7 +193,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -220,7 +220,7 @@ describe('#toExpression', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -243,11 +243,11 @@ describe('#toExpression', () => { }); }); - it('should default the valueLabels visibility settings to hide', () => { + it('should correctly report the valueLabels visibility settings', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: undefined }, + valueLabels: 'inside', preferredSeriesType: 'bar', layers: [ { @@ -261,8 +261,6 @@ describe('#toExpression', () => { }, frame.datasourceLayers ) as Ast; - expect((expression.chain[0].arguments.valueLabels[0] as Ast).chain[0].arguments).toEqual({ - mode: ['hide'], - }); + expect(expression.chain[0].arguments.valueLabels[0] as Ast).toEqual('inside'); }); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 31873b653fdbe..4bde104449443 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -44,7 +44,7 @@ export function toPreviewExpression( ...state.legend, isVisible: false, }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', }, datasourceLayers ); @@ -168,20 +168,7 @@ export const buildExpression = ( ], }, ], - valueLabels: [ - { - type: 'expression', - chain: [ - { - type: 'function', - function: 'lens_xy_valueLabelsConfig', - arguments: { - mode: [state?.valueLabels?.mode ?? 'hide'], - }, - }, - ], - }, - ], + valueLabels: [state?.valueLabels], layers: validLayers.map((layer) => { const columnToLabel: Record = {}; diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts index 53798df7c7747..526e9a762d330 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -78,35 +78,6 @@ export const legendConfig: ExpressionFunctionDefinition< }, }; -type ValueLabelsConfigResult = ValueLabelConfig & { - type: 'lens_xy_valueLabelsConfig'; -}; - -export const valueLabelsConfig: ExpressionFunctionDefinition< - 'lens_xy_valueLabelsConfig', - null, - ValueLabelConfig, - ValueLabelsConfigResult -> = { - name: 'lens_xy_valueLabelsConfig', - aliases: [], - type: 'lens_xy_valueLabelsConfig', - help: ``, - inputTypes: ['null'], - args: { - mode: { - types: ['string'], - help: '', - }, - }, - fn: function fn(input: unknown, args: ValueLabelConfig) { - return { - type: 'lens_xy_valueLabelsConfig', - ...args, - }; - }, -}; - export interface AxesSettingsConfig { x: boolean; yLeft: boolean; @@ -387,9 +358,7 @@ export type SeriesType = export type YAxisMode = 'auto' | 'left' | 'right'; -export interface ValueLabelConfig { - mode?: 'hide' | 'inside' | 'outside'; -} +export type ValueLabelConfig = 'hide' | 'inside' | 'outside'; export interface YConfig { forAccessor: string; @@ -426,7 +395,7 @@ export interface XYArgs { yTitle: string; yRightTitle: string; legend: LegendConfig & { type: 'lens_xy_legendConfig' }; - valueLabels: ValueLabelConfig & { type: 'lens_xy_valueLabelsConfig' }; + valueLabels: ValueLabelConfig; layers: LayerArgs[]; fittingFunction?: FittingFunction; axisTitlesVisibilitySettings?: AxesSettingsConfig & { diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts index 1c33411deda98..1daaea5777515 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -14,7 +14,7 @@ import { LensIconChartBar } from '../assets/chart_bar'; function exampleState(): State { return { legend: { position: Position.Bottom, isVisible: true }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -146,9 +146,7 @@ describe('xy_visualization', () => { }, "preferredSeriesType": "bar_stacked", "title": "Empty XY chart", - "valueLabels": Object { - "mode": "hide", - }, + "valueLabels": "hide", } `); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 6cfc0cd9d6442..0a2c0f412b8aa 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -140,7 +140,7 @@ export const xyVisualization: Visualization = { state || { title: 'Empty XY chart', legend: { isVisible: true, position: Position.Right }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: defaultSeriesType, layers: [ { diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 32dc45edb55b4..87bb268d59857 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -21,7 +21,7 @@ describe('XY Config panels', () => { function testState(): State { return { legend: { isVisible: true, position: Position.Right }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -127,7 +127,7 @@ describe('XY Config panels', () => { ...state, layers: [{ ...state.layers[0], seriesType: 'bar' }], fittingFunction: 'Carry', - valueLabels: { mode: 'inside' }, + valueLabels: 'inside', }} /> ); 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 index 82cbc24e76872..8a20886594cba 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -244,7 +244,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { ? 'hide' : 'show'; - const valueLabelsVisibilityMode = state?.valueLabels?.mode || 'hide'; + const valueLabelsVisibilityMode = state?.valueLabels || 'hide'; const tooltipContentValueLabels = isHistogramSeries ? valueTooltipContentDisabled.histogram : valueTooltipContentDisabled.stacked; @@ -287,7 +287,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { } onChange={(modeId) => { const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; - setState({ ...state, valueLabels: { mode: newMode } }); + setState({ ...state, valueLabels: newMode }); }} /> diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts index e88d67edf14d4..07f236c0f3bb8 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts @@ -131,7 +131,7 @@ describe('xy_suggestions', () => { keptLayerIds: [], state: { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -244,7 +244,7 @@ describe('xy_suggestions', () => { keptLayerIds: ['first'], state: { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -285,7 +285,7 @@ describe('xy_suggestions', () => { keptLayerIds: ['first', 'second'], state: { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -488,7 +488,7 @@ describe('xy_suggestions', () => { }, state: { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', layers: [ { @@ -541,7 +541,7 @@ describe('xy_suggestions', () => { test('keeps existing seriesType for initial tables', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', fittingFunction: 'None', preferredSeriesType: 'line', layers: [ @@ -575,7 +575,7 @@ describe('xy_suggestions', () => { test('makes a visible seriesType suggestion for unchanged table without split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, gridlinesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -616,7 +616,7 @@ describe('xy_suggestions', () => { test('suggests seriesType and stacking when there is a split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -662,7 +662,7 @@ describe('xy_suggestions', () => { (generateId as jest.Mock).mockReturnValueOnce('dummyCol'); const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', fittingFunction: 'None', preferredSeriesType: 'bar', layers: [ @@ -695,7 +695,7 @@ describe('xy_suggestions', () => { test('suggests stacking for unchanged table that has a split', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'None', layers: [ @@ -731,7 +731,7 @@ describe('xy_suggestions', () => { test('keeps column to dimension mappings on extended tables', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -774,7 +774,7 @@ describe('xy_suggestions', () => { test('changes column mappings when suggestion is reorder', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, @@ -818,7 +818,7 @@ describe('xy_suggestions', () => { (generateId as jest.Mock).mockReturnValueOnce('dummyCol'); const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, - valueLabels: { mode: 'hide' }, + valueLabels: 'hide', preferredSeriesType: 'bar', fittingFunction: 'None', axisTitlesVisibilitySettings: { x: true, yLeft: true, yRight: true }, diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts index e415124e8d516..307928575ed61 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts @@ -496,7 +496,7 @@ function buildSuggestion({ const state: State = { legend: currentState ? currentState.legend : { isVisible: true, position: Position.Right }, - valueLabels: currentState?.valueLabels || { mode: 'hide' }, + valueLabels: currentState?.valueLabels || 'hide', fittingFunction: currentState?.fittingFunction || 'None', xTitle: currentState?.xTitle, yTitle: currentState?.yTitle, From 47b104686b3387cd407c807eb3f70f7f415a6d24 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 12:29:44 +0100 Subject: [PATCH 09/21] :bug: Resolve backward compatibility --- x-pack/plugins/lens/public/xy_visualization/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts index 526e9a762d330..01748c03c94e9 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -409,7 +409,7 @@ export interface XYArgs { export interface XYState { preferredSeriesType: SeriesType; legend: LegendConfig; - valueLabels: ValueLabelConfig; + valueLabels?: ValueLabelConfig; fittingFunction?: FittingFunction; layers: LayerConfig[]; xTitle?: string; From 1119bf7c4822bfdd104cb1d805f42de502161aed Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 15:46:41 +0100 Subject: [PATCH 10/21] :lipstick: Dropdown always available with conditional disable logic for fields --- .../shared_components/toolbar_popover.tsx | 5 +- .../xy_visualization/xy_config_panel.scss | 4 + .../xy_visualization/xy_config_panel.tsx | 145 ++++++++++-------- 3 files changed, 89 insertions(+), 65 deletions(-) diff --git a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx index 679f3a44bb60e..da937450a030e 100644 --- a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx +++ b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx @@ -32,6 +32,7 @@ export interface ToolbarPopoverProps { */ groupPosition?: ToolbarButtonProps['groupPosition']; buttonDataTestSubj?: string; + larger: boolean; } export const ToolbarPopover: React.FunctionComponent = ({ @@ -41,15 +42,17 @@ export const ToolbarPopover: React.FunctionComponent = ({ isDisabled = false, groupPosition, buttonDataTestSubj, + larger, }) => { const [open, setOpen] = useState(false); const iconType: string | IconType = typeof type === 'string' ? typeToIconMap[type] : type; + const panelClassName = larger ? 'lnsXyToolbar__popoverLarger' : 'lnsXyToolbar__popover'; return ( ) { ? valueTooltipContentDisabled.histogram : valueTooltipContentDisabled.stacked; + const isValueLabelsDisabled = hasNonBarSeries || !hasBarNotStacked || isHistogramSeries; + return ( - - + {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { + defaultMessage: 'Display', + })}{' '} + {isValueLabelsDisabled ? ( + + ) : null} + + } > - - value === valueLabelsVisibilityMode)!.id + } + onChange={(modeId) => { + const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; + setState({ ...state, valueLabels: newMode }); + }} + /> + + value === valueLabelsVisibilityMode)!.id - } - onChange={(modeId) => { - const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; - setState({ ...state, valueLabels: newMode }); - }} - /> - - + {i18n.translate('xpack.lens.xyChart.missingValuesLabel', { + defaultMessage: 'Missing values', + })}{' '} + {!hasNonBarSeries ? : null} + + } + > + { + return { + value: id, + dropdownDisplay: ( + <> + {title} + +

{description}

+ + + ), + inputDisplay: title, + }; })} - > - { - return { - value: id, - dropdownDisplay: ( - <> - {title} - -

{description}

- - - ), - inputDisplay: title, - }; - })} - valueOfSelected={state?.fittingFunction || 'None'} - onChange={(value) => setState({ ...state, fittingFunction: value })} - itemLayoutAlign="top" - hasDividers - /> - - - + valueOfSelected={state?.fittingFunction || 'None'} + onChange={(value) => setState({ ...state, fittingFunction: value })} + itemLayoutAlign="top" + hasDividers + /> + + Date: Mon, 2 Nov 2020 16:38:09 +0100 Subject: [PATCH 11/21] :label: Fix type issues --- .../plugins/lens/public/shared_components/toolbar_popover.tsx | 2 +- x-pack/plugins/lens/public/xy_visualization/expression.tsx | 2 +- x-pack/plugins/lens/public/xy_visualization/to_expression.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx index da937450a030e..8c9bbd4085179 100644 --- a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx +++ b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx @@ -32,7 +32,7 @@ export interface ToolbarPopoverProps { */ groupPosition?: ToolbarButtonProps['groupPosition']; buttonDataTestSubj?: string; - larger: boolean; + larger?: boolean; } export const ToolbarPopover: React.FunctionComponent = ({ diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 2e7e9318e8f33..33cbe3646a3fe 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -745,7 +745,7 @@ export function XYChart({ // This format double fixes two issues in elastic-chart // * when rotating the chart, the formatter is not correctly picked // * in some scenarios value labels are not strings, and this breaks the elastic-chart lib - valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', + // valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', showValueLabel: shouldShowValueLabels && valueLabels !== 'hide', isAlternatingValueLabel: false, isValueContainedInElement: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 4bde104449443..19d14c5a17a59 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -168,7 +168,7 @@ export const buildExpression = ( ], }, ], - valueLabels: [state?.valueLabels], + valueLabels: [state?.valueLabels || 'hide'], layers: validLayers.map((layer) => { const columnToLabel: Record = {}; From 4a235c69919afee99538f02ab335a4b5484be2c3 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 17:00:02 +0100 Subject: [PATCH 12/21] :white_check_mark: Fix tests with new UX/UI --- .../public/xy_visualization/expression.tsx | 2 +- .../xy_visualization/xy_config_panel.test.tsx | 30 +++++++++++++++---- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 33cbe3646a3fe..2e7e9318e8f33 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -745,7 +745,7 @@ export function XYChart({ // This format double fixes two issues in elastic-chart // * when rotating the chart, the formatter is not correctly picked // * in some scenarios value labels are not strings, and this breaks the elastic-chart lib - // valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', + valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', showValueLabel: shouldShowValueLabels && valueLabels !== 'hide', isAlternatingValueLabel: false, isValueContainedInElement: true, diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 87bb268d59857..99c2a1fe79be2 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -135,7 +135,7 @@ describe('XY Config panels', () => { expect(component.find(EuiButtonGroup).prop('idSelected')).toEqual('value_labels_inside'); }); - it('should disable the popover if there is no area, line, bar or bar_horizontal series', () => { + it('should show the popover, but disable the fitting field if there is no area, line, bar or bar_horizontal series', () => { const state = testState(); const component = shallow( { /> ); - expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(true); + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsMissingValuesSelect"]') + .prop('disabled') + ).toEqual(true); }); - it('should disable the popover if there is histogram series', () => { + it('should show the popover, but disabled the display field if there is histogram series', () => { // make it detect an histogram series frame.datasourceLayers.first.getOperationForColumnId = jest.fn().mockReturnValueOnce({ isBucketed: true, @@ -169,10 +175,16 @@ describe('XY Config panels', () => { /> ); - expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(true); + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsValueLabelsDisplay"]') + .prop('isDisabled') + ).toEqual(true); }); - it('should show the popover for bar and horizontal_bar series', () => { + it('should show the popover and display field enabled for bar and horizontal_bar series', () => { const state = testState(); const component = shallow( @@ -187,7 +199,13 @@ describe('XY Config panels', () => { /> ); - expect(component.find(ToolbarPopover).at(0).prop('isDisabled')).toEqual(false); + expect( + component + .find(ToolbarPopover) + .at(0) + .find('[data-test-subj="lnsValueLabelsDisplay"]') + .prop('isDisabled') + ).toEqual(false); }); it('should disable the fitting option for bar series', () => { From 7ca4e342ff42816b039f39e9b929f9e6515a4f19 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 18:31:02 +0100 Subject: [PATCH 13/21] :lipstick: Update value label border styling --- x-pack/plugins/lens/public/xy_visualization/expression.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 2e7e9318e8f33..4dbe0de5eccf8 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -235,7 +235,7 @@ function mergeThemeWithValueLabelsStyling( ...theme.barSeriesStyle, displayValue: { fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE }, - fill: { textInverted: true, textBorder: true }, + fill: { textInverted: true, textBorder: 2 }, alignment: isHorizontal ? { vertical: VerticalAlignment.Middle, From 25522d53c51f1ce656372fdd30cc70394d5f8b9d Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 3 Nov 2020 12:15:36 +0100 Subject: [PATCH 14/21] :globe_with_meridians: + :ok_hand: Revisited messages + remove unused value --- .../public/xy_visualization/expression.tsx | 2 +- .../xy_visualization/xy_config_panel.tsx | 25 +++++++------------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/expression.tsx b/x-pack/plugins/lens/public/xy_visualization/expression.tsx index 4dbe0de5eccf8..b8bcd10d800e7 100644 --- a/x-pack/plugins/lens/public/xy_visualization/expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/expression.tsx @@ -438,7 +438,7 @@ export function XYChart({ const baseThemeWithMaybeValueLabels = !shouldShowValueLabels ? chartTheme - : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels || 'hide', shouldRotate); + : mergeThemeWithValueLabelsStyling(chartTheme, valueLabels, shouldRotate); return ( 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 index 0e5e0d41c34fe..8a61221649e65 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -108,16 +108,6 @@ const valueLabelsOptions: Array<{ }, ]; -const valueTooltipContentDisabled = { - stacked: i18n.translate('xpack.lens.xyChart.valuesStackedDisabledHelpText', { - defaultMessage: - 'This setting cannot be applied to stacked bar charts. Try other type of charts', - }), - histogram: i18n.translate('xpack.lens.xyChart.valuesDisabledHelpText', { - defaultMessage: 'This setting cannot be applied to bar charts having time dimension.', - }), -}; - export function LayerContextMenu(props: VisualizationLayerWidgetProps) { const { state, layerId } = props; const horizontalOnly = isHorizontalChart(state.layers); @@ -245,9 +235,6 @@ export function XyToolbar(props: VisualizationToolbarProps) { : 'show'; const valueLabelsVisibilityMode = state?.valueLabels || 'hide'; - const tooltipContentValueLabels = isHistogramSeries - ? valueTooltipContentDisabled.histogram - : valueTooltipContentDisabled.stacked; const isValueLabelsDisabled = hasNonBarSeries || !hasBarNotStacked || isHistogramSeries; @@ -268,11 +255,17 @@ export function XyToolbar(props: VisualizationToolbarProps) { display="columnCompressed" label={ {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { - defaultMessage: 'Display', + defaultMessage: 'Labels', })}{' '} {isValueLabelsDisabled ? ( @@ -283,7 +276,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { Date: Tue, 3 Nov 2020 14:15:01 +0100 Subject: [PATCH 15/21] :world_with_meridians: fix i18n issue --- x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 8a61221649e65..e679f754fdbf6 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -275,7 +275,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { > Date: Wed, 4 Nov 2020 11:54:19 +0100 Subject: [PATCH 16/21] :lipstick: Hide missing fields row on bar charts + better icon alignment --- .../shared_components/toolbar_popover.tsx | 5 +- .../xy_visualization/xy_config_panel.scss | 4 - .../xy_visualization/xy_config_panel.tsx | 88 +++++++++---------- 3 files changed, 44 insertions(+), 53 deletions(-) diff --git a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx index 8c9bbd4085179..679f3a44bb60e 100644 --- a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx +++ b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx @@ -32,7 +32,6 @@ export interface ToolbarPopoverProps { */ groupPosition?: ToolbarButtonProps['groupPosition']; buttonDataTestSubj?: string; - larger?: boolean; } export const ToolbarPopover: React.FunctionComponent = ({ @@ -42,17 +41,15 @@ export const ToolbarPopover: React.FunctionComponent = ({ isDisabled = false, groupPosition, buttonDataTestSubj, - larger, }) => { const [open, setOpen] = useState(false); const iconType: string | IconType = typeof type === 'string' ? typeToIconMap[type] : type; - const panelClassName = larger ? 'lnsXyToolbar__popoverLarger' : 'lnsXyToolbar__popover'; return ( ) { )} condition={isValueLabelsDisabled} > - {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { - defaultMessage: 'Labels', - })}{' '} - {isValueLabelsDisabled ? ( - - ) : null} + + {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { + defaultMessage: 'Labels', + })}{' '} + {isValueLabelsDisabled ? ( + + ) : null} + } > @@ -292,46 +299,37 @@ export function XyToolbar(props: VisualizationToolbarProps) { }} /> - - {i18n.translate('xpack.lens.xyChart.missingValuesLabel', { - defaultMessage: 'Missing values', - })}{' '} - {!hasNonBarSeries ? : null} - - } - > - { - return { - value: id, - dropdownDisplay: ( - <> - {title} - -

{description}

- - - ), - inputDisplay: title, - }; + {hasNonBarSeries ? ( + setState({ ...state, fittingFunction: value })} - itemLayoutAlign="top" - hasDividers - /> - + > + { + return { + value: id, + dropdownDisplay: ( + <> + {title} + +

{description}

+ + + ), + inputDisplay: title, + }; + })} + valueOfSelected={state?.fittingFunction || 'None'} + onChange={(value) => setState({ ...state, fittingFunction: value })} + itemLayoutAlign="top" + hasDividers + /> + + ) : null} Date: Wed, 4 Nov 2020 12:14:24 +0100 Subject: [PATCH 17/21] :fire: Remove old cruft --- x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx | 1 - 1 file changed, 1 deletion(-) 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 index 5b7b168aad484..1b44e47083235 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -250,7 +250,6 @@ export function XyToolbar(props: VisualizationToolbarProps) { type="values" groupPosition="left" buttonDataTestSubj="lnsMissingValuesButton" - larger > Date: Wed, 4 Nov 2020 12:57:52 +0100 Subject: [PATCH 18/21] :white_check_mark: Align tests to new UX --- .../xy_visualization/xy_config_panel.test.tsx | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 99c2a1fe79be2..b20510cd1f167 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -14,6 +14,7 @@ import { FramePublicAPI } from '../types'; import { State } from './types'; import { Position } from '@elastic/charts'; import { createMockFramePublicAPI, createMockDatasource } from '../editor_frame_service/mocks'; +import { componentDetailsFlyoutProps } from '../../../index_management/public/application/components'; describe('XY Config panels', () => { let frame: FramePublicAPI; @@ -135,7 +136,7 @@ describe('XY Config panels', () => { expect(component.find(EuiButtonGroup).prop('idSelected')).toEqual('value_labels_inside'); }); - it('should show the popover, but disable the fitting field if there is no area, line, bar or bar_horizontal series', () => { + it('should show the popover, but hide the fitting field if there is no area, line series', () => { const state = testState(); const component = shallow( { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsMissingValuesSelect"]') - .prop('disabled') - ).toEqual(true); + expect(component.exists('[data-test-subj="lnsMissingValuesSelect"]')).toEqual(false); }); it('should show the popover, but disabled the display field if there is histogram series', () => { @@ -208,7 +203,7 @@ describe('XY Config panels', () => { ).toEqual(false); }); - it('should disable the fitting option for bar series', () => { + it('should hide the fitting option for bar series', () => { const state = testState(); const component = shallow( { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsMissingValuesSelect"]') - .prop('disabled') - ).toEqual(true); + expect(component.exists('[data-test-subj="lnsMissingValuesSelect"]')).toEqual(false); }); it('should disable the display option for area and line series', () => { From b96c18869e2f5f67bfcdb2bf8a168ccf294754d2 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 13:23:42 +0100 Subject: [PATCH 19/21] :label: Fix type issue --- .../lens/public/xy_visualization/xy_config_panel.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index b20510cd1f167..42105b7832d09 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -14,7 +14,6 @@ import { FramePublicAPI } from '../types'; import { State } from './types'; import { Position } from '@elastic/charts'; import { createMockFramePublicAPI, createMockDatasource } from '../editor_frame_service/mocks'; -import { componentDetailsFlyoutProps } from '../../../index_management/public/application/components'; describe('XY Config panels', () => { let frame: FramePublicAPI; From d9562aa73e504de7605825d7b0a6d35437ba3106 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Nov 2020 18:59:35 +0100 Subject: [PATCH 20/21] :ok_hand: Implemented review feedback --- .../shared_components/toolbar_popover.tsx | 2 +- .../xy_visualization/xy_config_panel.test.tsx | 56 +++--- .../xy_visualization/xy_config_panel.tsx | 176 +++++++++--------- .../test/functional/page_objects/lens_page.ts | 4 +- 4 files changed, 116 insertions(+), 122 deletions(-) diff --git a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx index 679f3a44bb60e..20837424dc7b5 100644 --- a/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx +++ b/x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx @@ -11,7 +11,7 @@ import { EuiIconLegend } from '../assets/legend'; const typeToIconMap: { [type: string]: string | IconType } = { legend: EuiIconLegend as IconType, - values: 'visText', + values: 'number', }; export interface ToolbarPopoverProps { diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx index 42105b7832d09..721bff8684a19 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.test.tsx @@ -135,7 +135,7 @@ describe('XY Config panels', () => { expect(component.find(EuiButtonGroup).prop('idSelected')).toEqual('value_labels_inside'); }); - it('should show the popover, but hide the fitting field if there is no area, line series', () => { + it('should disable the popover for stacked bar charts', () => { const state = testState(); const component = shallow( { /> ); - expect(component.exists('[data-test-subj="lnsMissingValuesSelect"]')).toEqual(false); + expect(component.find(ToolbarPopover).prop('isDisabled')).toEqual(true); + }); + + it('should disable the popover for percentage area charts', () => { + const state = testState(); + const component = shallow( + + ); + + expect(component.find(ToolbarPopover).prop('isDisabled')).toEqual(true); }); - it('should show the popover, but disabled the display field if there is histogram series', () => { + it('should disabled the popover if there is histogram series', () => { // make it detect an histogram series frame.datasourceLayers.first.getOperationForColumnId = jest.fn().mockReturnValueOnce({ isBucketed: true, @@ -169,13 +185,7 @@ describe('XY Config panels', () => { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsValueLabelsDisplay"]') - .prop('isDisabled') - ).toEqual(true); + expect(component.find(ToolbarPopover).prop('isDisabled')).toEqual(true); }); it('should show the popover and display field enabled for bar and horizontal_bar series', () => { @@ -193,13 +203,7 @@ describe('XY Config panels', () => { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsValueLabelsDisplay"]') - .prop('isDisabled') - ).toEqual(false); + expect(component.exists('[data-test-subj="lnsValueLabelsDisplay"]')).toEqual(true); }); it('should hide the fitting option for bar series', () => { @@ -219,7 +223,7 @@ describe('XY Config panels', () => { expect(component.exists('[data-test-subj="lnsMissingValuesSelect"]')).toEqual(false); }); - it('should disable the display option for area and line series', () => { + it('should hide in the popover the display option for area and line series', () => { const state = testState(); const component = shallow( { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsValueLabelsDisplay"]') - .prop('isDisabled') - ).toEqual(true); + expect(component.exists('[data-test-subj="lnsValueLabelsDisplay"]')).toEqual(false); }); it('should keep the display option for bar series with multiple layers', () => { @@ -270,13 +268,7 @@ describe('XY Config panels', () => { /> ); - expect( - component - .find(ToolbarPopover) - .at(0) - .find('[data-test-subj="lnsValueLabelsDisplay"]') - .prop('isDisabled') - ).toEqual(false); + expect(component.exists('[data-test-subj="lnsValueLabelsDisplay"]')).toEqual(true); }); it('should disable the popover if there is no right axis', () => { 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 index 1b44e47083235..757586b0d5d58 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -86,8 +86,6 @@ const legendOptions: Array<{ id: string; value: 'auto' | 'show' | 'hide'; label: }, ]; -// TODO: inside/outside logic requires some more work on the elastic-chart side -// as for now limit to a show/hide logic const valueLabelsOptions: Array<{ id: string; value: 'hide' | 'inside' | 'outside'; @@ -163,6 +161,10 @@ export function XyToolbar(props: VisualizationToolbarProps) { ['bar', 'bar_horizontal'].includes(seriesType) ); + const isAreaPercentage = state?.layers.some( + ({ seriesType }) => seriesType === 'area_percentage_stacked' + ); + const isHistogramSeries = Boolean( hasHistogramSeries(state?.layers as ValidLayer[], frame.datasourceLayers) ); @@ -237,100 +239,100 @@ export function XyToolbar(props: VisualizationToolbarProps) { const valueLabelsVisibilityMode = state?.valueLabels || 'hide'; - const isValueLabelsDisabled = hasNonBarSeries || !hasBarNotStacked || isHistogramSeries; + const isValueLabelsEnabled = !hasNonBarSeries && hasBarNotStacked && !isHistogramSeries; + const isFittingEnabled = hasNonBarSeries; return ( - - + {isValueLabelsEnabled ? ( + + {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { + defaultMessage: 'Labels', + })} + + } > - - {i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { + - ) : null} - - - } - > - value === valueLabelsVisibilityMode)!.id - } - onChange={(modeId) => { - const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; - setState({ ...state, valueLabels: newMode }); - }} - /> - - {hasNonBarSeries ? ( - - { - return { - value: id, - dropdownDisplay: ( - <> - {title} - -

{description}

- - - ), - inputDisplay: title, - }; + })} + data-test-subj="lnsValueLabelsDisplay" + name="valueLabelsDisplay" + buttonSize="compressed" + options={valueLabelsOptions} + idSelected={ + valueLabelsOptions.find(({ value }) => value === valueLabelsVisibilityMode)! + .id + } + onChange={(modeId) => { + const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; + setState({ ...state, valueLabels: newMode }); + }} + /> + + ) : null} + {isFittingEnabled ? ( + setState({ ...state, fittingFunction: value })} - itemLayoutAlign="top" - hasDividers - /> - - ) : null} - + > + { + return { + value: id, + dropdownDisplay: ( + <> + {title} + +

{description}

+ + + ), + inputDisplay: title, + }; + })} + valueOfSelected={state?.fittingFunction || 'None'} + onChange={(value) => setState({ ...state, fittingFunction: value })} + itemLayoutAlign="top" + hasDividers + /> + + ) : null} + + { - await testSubjects.click('lnsMissingValuesButton'); - await testSubjects.exists('lnsMissingValuesSelect'); + await testSubjects.click('lnsValuesButton'); + await testSubjects.exists('lnsValuesButton'); }); await testSubjects.click('lnsMissingValuesSelect'); const optionSelector = await find.byCssSelector(`#${option}`); From 036bbde8c7f32593a13a32fa39b43f1c4d1b75e1 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 6 Nov 2020 10:37:05 +0100 Subject: [PATCH 21/21] :world_with_meridians: Change copy messages --- .../lens/public/xy_visualization/xy_config_panel.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 index 757586b0d5d58..a22530c5743b4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -250,12 +250,10 @@ export function XyToolbar(props: VisualizationToolbarProps) { tooltipContent={ isAreaPercentage ? i18n.translate('xpack.lens.xyChart.valuesPercentageDisabledHelpText', { - defaultMessage: - 'This setting may be modified on area and stacked area charts, but not percentage area charts.', + defaultMessage: 'This setting cannot be changed on percentage area charts.', }) : i18n.translate('xpack.lens.xyChart.valuesStackedDisabledHelpText', { - defaultMessage: - 'This setting may be modified on categorical bar charts, but not histograms', + defaultMessage: 'This setting cannot be changed on histograms.', }) } condition={!isValueLabelsEnabled && !isFittingEnabled}