From 0d001d5b2a614ac4064b22715718d6b2e750de65 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Wed, 15 Jul 2020 12:11:08 +0300 Subject: [PATCH 01/14] [Lens] Add styling options for x axis on the settings popover --- .../__snapshots__/to_expression.test.ts.snap | 9 ++ .../__snapshots__/xy_expression.test.tsx.snap | 42 +++++++-- .../xy_visualization/to_expression.test.ts | 23 +++++ .../public/xy_visualization/to_expression.ts | 5 +- .../lens/public/xy_visualization/types.ts | 7 ++ .../xy_visualization/xy_config_panel.test.tsx | 46 ++++++++++ .../xy_visualization/xy_config_panel.tsx | 85 +++++++++++++++++ .../xy_visualization/xy_expression.test.tsx | 92 +++++++++++++++++++ .../public/xy_visualization/xy_expression.tsx | 31 ++++++- .../xy_visualization/xy_suggestions.test.ts | 15 +++ .../public/xy_visualization/xy_suggestions.ts | 4 + 11 files changed, 346 insertions(+), 13 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 d7d76bdd1f44a..7c32383c02e2e 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 @@ -8,6 +8,12 @@ Object { "fittingFunction": Array [ "Carry", ], + "hideXAxisTickLabels": Array [ + false, + ], + "hideXAxisTitle": Array [ + false, + ], "layers": Array [ Object { "chain": Array [ @@ -72,6 +78,9 @@ Object { "type": "expression", }, ], + "showXAxisGridlines": Array [ + true, + ], "xTitle": Array [ "col_a", ], diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap index c7c173f87ad7c..d8b58b29cfcd1 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap @@ -20,9 +20,13 @@ exports[`xy_expression XYChart component it renders area 1`] = ` } /> @@ -146,9 +150,13 @@ exports[`xy_expression XYChart component it renders bar 1`] = ` } /> @@ -262,9 +270,13 @@ exports[`xy_expression XYChart component it renders horizontal bar 1`] = ` } /> @@ -378,9 +390,13 @@ exports[`xy_expression XYChart component it renders line 1`] = ` } /> @@ -504,9 +520,13 @@ exports[`xy_expression XYChart component it renders stacked area 1`] = ` } /> @@ -628,9 +648,13 @@ exports[`xy_expression XYChart component it renders stacked bar 1`] = ` } /> @@ -752,9 +776,13 @@ exports[`xy_expression XYChart component it renders stacked horizontal bar 1`] = } /> 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 31b34e41e82db..95676f3e59493 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 @@ -41,6 +41,7 @@ describe('#toExpression', () => { legend: { position: Position.Bottom, isVisible: true }, preferredSeriesType: 'bar', fittingFunction: 'Carry', + showXAxisGridlines: true, layers: [ { layerId: 'first', @@ -77,6 +78,28 @@ describe('#toExpression', () => { ).toEqual('None'); }); + it('should default the hideXAxisTitle, showXAxisGridlines and hideXAxisTickLabels to false', () => { + const expression = xyVisualization.toExpression( + { + legend: { position: Position.Bottom, isVisible: true }, + preferredSeriesType: 'bar', + layers: [ + { + layerId: 'first', + seriesType: 'area', + splitAccessor: 'd', + xAccessor: 'a', + accessors: ['b', 'c'], + }, + ], + }, + frame + ) as Ast; + expect(expression.chain[0].arguments.hideXAxisTitle[0]).toBeFalsy(); + expect(expression.chain[0].arguments.showXAxisGridlines[0]).toBeFalsy(); + expect(expression.chain[0].arguments.hideXAxisTickLabels[0]).toBeFalsy(); + }); + it('should not generate an expression when missing x', () => { expect( xyVisualization.toExpression( 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 3b9406cedd499..8c1ac19731606 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -116,7 +116,7 @@ export const buildExpression = ( type: 'function', function: 'lens_xy_chart', arguments: { - xTitle: [xTitle], + xTitle: [state.xTitle || xTitle], yTitle: [yTitle], legend: [ { @@ -134,6 +134,9 @@ export const buildExpression = ( }, ], fittingFunction: [state.fittingFunction || 'None'], + hideXAxisTitle: [state.hideXAxisTitle || false], + showXAxisGridlines: [state.showXAxisGridlines || false], + hideXAxisTickLabels: [state.hideXAxisTickLabels || false], 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 08f29c65b26d0..7f85f1ce8dfa1 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -227,6 +227,9 @@ export interface XYArgs { legend: LegendConfig & { type: 'lens_xy_legendConfig' }; layers: LayerArgs[]; fittingFunction?: FittingFunction; + hideXAxisTitle?: boolean; + showXAxisGridlines?: boolean; + hideXAxisTickLabels?: boolean; } // Persisted parts of the state @@ -235,6 +238,10 @@ export interface XYState { legend: LegendConfig; fittingFunction?: FittingFunction; layers: LayerConfig[]; + xTitle?: string; + hideXAxisTitle?: boolean; + showXAxisGridlines?: boolean; + hideXAxisTickLabels?: boolean; } export type State = XYState; 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 981ce1cca5958..770ad712cae21 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 @@ -127,5 +127,51 @@ describe('XY Config panels', () => { expect(component.find(EuiSuperSelect).prop('disabled')).toEqual(true); }); + + it('should show the hideXAxisTitle, hideXAxisTickLabels and showXAxisGridlines Switches on', () => { + const state = testState(); + + const component = shallow( + + ); + + expect( + component.find('[data-test-subj="lnsHideXAxisTitleSwitch"]').prop('checked') + ).toBeTruthy(); + expect( + component.find('[data-test-subj="lnsHideXAxisTickLabelsSwitch"]').prop('checked') + ).toBeTruthy(); + expect( + component.find('[data-test-subj="lnsShowXAxisGridLinesSwitch"]').prop('checked') + ).toBeTruthy(); + }); + + it('should show the value of the X axis title on the corresponding input text', () => { + const state = testState(); + + const component = shallow( + + ); + + expect(component.find('[data-test-subj="lnsXAxisTitle"]').prop('value')).toBe( + 'My custom X axis title' + ); + }); }); }); 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 d22b3ec0a44a6..fc314942df63c 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 @@ -22,6 +22,10 @@ import { EuiColorPickerProps, EuiToolTip, EuiIcon, + EuiFieldText, + EuiSwitch, + EuiHorizontalRule, + EuiTitle, } from '@elastic/eui'; import { VisualizationLayerWidgetProps, @@ -95,6 +99,13 @@ export function XyToolbar(props: VisualizationToolbarProps) { const hasNonBarSeries = props.state?.layers.some( (layer) => layer.seriesType === 'line' || layer.seriesType === 'area' ); + const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); + + const onXTitleChange = (value: string) => { + setXtitle(value); + props.setState({ ...props.state, xTitle: value }); + }; + return ( @@ -157,6 +168,80 @@ export function XyToolbar(props: VisualizationToolbarProps) { /> + + + X Axis + + + onXTitleChange(target.value)} + aria-label="Overwrite X Axis Title" + /> + + + + props.setState({ ...props.state, hideXAxisTitle: target.checked }) + } + checked={props.state?.hideXAxisTitle || false} + /> + + + + props.setState({ ...props.state, hideXAxisTickLabels: target.checked }) + } + checked={props.state?.hideXAxisTickLabels || false} + /> + + + + props.setState({ ...props.state, showXAxisGridlines: target.checked }) + } + checked={props.state?.showXAxisGridlines || false} + /> + + diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx index b7a50b3af640c..18294e2575d05 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx @@ -1365,6 +1365,35 @@ describe('xy_expression', () => { expect(convertSpy).toHaveBeenCalledWith('I'); }); + test('it should not pass the formatter function to the axis if the hideXAxisTickLabels switch is on', () => { + const { data, args } = sampleArgs(); + + args.hideXAxisTickLabels = true; + + const instance = shallow( + + ); + + const tickFormatter = instance.find(Axis).first().prop('tickFormat'); + + if (!tickFormatter) { + throw new Error('tickFormatter prop not found'); + } + + tickFormatter('I'); + + expect(convertSpy).toHaveBeenCalledTimes(0); + }); + test('it should remove invalid rows', () => { const data: LensMultiTable = { type: 'lens_multitable', @@ -1616,5 +1645,68 @@ describe('xy_expression', () => { expect(component.find(LineSeries).prop('fit')).toEqual({ type: Fit.None }); }); + + test('it should apply the xTitle if is specified', () => { + const { data, args } = sampleArgs(); + + args.xTitle = 'My custom x-axis title'; + + const component = shallow( + + ); + + expect(component.find(Axis).at(0).prop('title')).toEqual('My custom x-axis title'); + }); + + test('it should hide the X axis title if the corresponding switch is on', () => { + const { data, args } = sampleArgs(); + + args.hideXAxisTitle = true; + + const component = shallow( + + ); + + expect(component.find(Axis).at(0).prop('title')).toEqual(undefined); + }); + + test('it should show the X axis gridlines if the corresponding switch is on', () => { + const { data, args } = sampleArgs(); + + args.showXAxisGridlines = true; + + const component = shallow( + + ); + + expect(component.find(Axis).at(0).prop('showGridLines')).toBeTruthy(); + }); }); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 3ab12aa0879b0..23c840523361f 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -102,6 +102,18 @@ export const xyChart: ExpressionFunctionDefinition< defaultMessage: 'Define how missing values are treated', }), }, + hideXAxisTitle: { + types: ['boolean'], + help: 'Hide X axis title', + }, + showXAxisGridlines: { + types: ['boolean'], + help: 'Show X axis gridlines', + }, + hideXAxisTickLabels: { + types: ['boolean'], + help: 'Hide X axis tick labels', + }, layers: { // eslint-disable-next-line @typescript-eslint/no-explicit-any types: ['lens_xy_layer'] as any, @@ -199,7 +211,14 @@ export function XYChart({ onClickValue, onSelectRange, }: XYChartRenderProps) { - const { legend, layers, fittingFunction } = args; + const { + legend, + layers, + fittingFunction, + hideXAxisTitle, + showXAxisGridlines, + hideXAxisTickLabels, + } = args; const chartTheme = chartsThemeService.useChartsTheme(); const chartBaseTheme = chartsThemeService.useChartsBaseTheme(); @@ -237,7 +256,7 @@ export function XYChart({ shouldRotate ); - const xTitle = (xAxisColumn && xAxisColumn.name) || args.xTitle; + const xTitle = args.xTitle || (xAxisColumn && xAxisColumn.name); function calculateMinInterval() { // check all the tables to see if all of the rows have the same timestamp @@ -373,10 +392,12 @@ export function XYChart({ xAxisFormatter.convert(d)} + // @ts-ignore + tickFormat={!hideXAxisTickLabels ? (d) => xAxisFormatter.convert(d) : () => {}} /> {yAxesConfiguration.map((axis, index) => ( 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 f301206355060..aae17a2f47f43 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 @@ -332,6 +332,9 @@ describe('xy_suggestions', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, fittingFunction: 'None', + hideXAxisTitle: false, + showXAxisGridlines: false, + hideXAxisTickLabels: false, preferredSeriesType: 'bar', layers: [ { @@ -370,6 +373,9 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', + hideXAxisTitle: false, + showXAxisGridlines: false, + hideXAxisTickLabels: false, layers: [ { accessors: ['price', 'quantity'], @@ -479,6 +485,9 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', + hideXAxisTitle: false, + showXAxisGridlines: false, + hideXAxisTickLabels: false, layers: [ { accessors: ['price', 'quantity'], @@ -518,6 +527,9 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', + hideXAxisTitle: false, + showXAxisGridlines: false, + hideXAxisTickLabels: false, layers: [ { accessors: ['price'], @@ -558,6 +570,9 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', + hideXAxisTitle: false, + showXAxisGridlines: false, + hideXAxisTickLabels: false, layers: [ { accessors: ['price', 'quantity'], 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 e0bfbd266f8f1..6b52cab12d1c4 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts @@ -403,6 +403,10 @@ function buildSuggestion({ const state: State = { legend: currentState ? currentState.legend : { isVisible: true, position: Position.Right }, fittingFunction: currentState?.fittingFunction || 'None', + xTitle: currentState?.xTitle, + hideXAxisTitle: currentState?.hideXAxisTitle || false, + showXAxisGridlines: currentState?.showXAxisGridlines || false, + hideXAxisTickLabels: currentState?.hideXAxisTickLabels || false, preferredSeriesType: seriesType, layers: [...keptLayers, newLayer], }; From 2c1ec95d26bccce36e263b8052f2b31865275c57 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Wed, 15 Jul 2020 17:08:28 +0300 Subject: [PATCH 02/14] ts related changes --- .../plugins/lens/public/xy_visualization/xy_config_panel.tsx | 4 ++-- x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx | 2 +- 2 files changed, 3 insertions(+), 3 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 fc314942df63c..fefe308b6e8e1 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 @@ -99,9 +99,9 @@ export function XyToolbar(props: VisualizationToolbarProps) { const hasNonBarSeries = props.state?.layers.some( (layer) => layer.seriesType === 'line' || layer.seriesType === 'area' ); - const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); + const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); - const onXTitleChange = (value: string) => { + const onXTitleChange = (value: string): void => { setXtitle(value); props.setState({ ...props.state, xTitle: value }); }; diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 23c840523361f..2285aa6fa6828 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -396,7 +396,7 @@ export function XYChart({ showGridLines={showXAxisGridlines} gridLineStyle={{ strokeWidth: 2 }} hide={filteredLayers[0].hide} - // @ts-ignore + // @ts-ignore, temporary solution for not displaying the ticks tickFormat={!hideXAxisTickLabels ? (d) => xAxisFormatter.convert(d) : () => {}} /> From 4235afb5916a7aedf2ae25991d237b524b87a351 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Thu, 16 Jul 2020 18:56:29 +0300 Subject: [PATCH 03/14] Changes to the popover's design and y-axis implementatin --- .../__snapshots__/to_expression.test.ts.snap | 47 ++++- .../__snapshots__/xy_expression.test.tsx.snap | 14 +- .../lens/public/xy_visualization/index.ts | 4 +- .../xy_visualization/to_expression.test.ts | 60 +++++- .../public/xy_visualization/to_expression.ts | 45 +++- .../lens/public/xy_visualization/types.ts | 89 +++++++- .../xy_visualization/xy_config_panel.test.tsx | 32 ++- .../xy_visualization/xy_config_panel.tsx | 198 +++++++++++++----- .../xy_visualization/xy_expression.test.tsx | 72 ++++++- .../public/xy_visualization/xy_expression.tsx | 54 +++-- .../xy_visualization/xy_suggestions.test.ts | 35 ++-- .../public/xy_visualization/xy_suggestions.ts | 14 +- 12 files changed, 520 insertions(+), 144 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 7c32383c02e2e..10d17572c8962 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 @@ -8,11 +8,24 @@ Object { "fittingFunction": Array [ "Carry", ], - "hideXAxisTickLabels": Array [ - false, - ], - "hideXAxisTitle": Array [ - false, + "gridlinesVisibilitySettings": Array [ + Object { + "chain": Array [ + Object { + "arguments": Object { + "x": Array [ + false, + ], + "y": Array [ + true, + ], + }, + "function": "lens_xy_gridlinesConfig", + "type": "function", + }, + ], + "type": "expression", + }, ], "layers": Array [ Object { @@ -78,9 +91,31 @@ Object { "type": "expression", }, ], - "showXAxisGridlines": Array [ + "showXAxisTitle": Array [ + true, + ], + "showYAxisTitle": Array [ true, ], + "tickLabelsVisibilitySettings": Array [ + Object { + "chain": Array [ + Object { + "arguments": Object { + "x": Array [ + false, + ], + "y": Array [ + true, + ], + }, + "function": "lens_xy_tickLabelsConfig", + "type": "function", + }, + ], + "type": "expression", + }, + ], "xTitle": Array [ "col_a", ], diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap index d8b58b29cfcd1..61a90fc88fa26 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap @@ -27,6 +27,7 @@ exports[`xy_expression XYChart component it renders area 1`] = ` } id="x" position="bottom" + showGridLines={true} tickFormat={[Function]} title="c" /> @@ -37,7 +38,6 @@ exports[`xy_expression XYChart component it renders area 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -167,7 +168,6 @@ exports[`xy_expression XYChart component it renders bar 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -287,7 +288,6 @@ exports[`xy_expression XYChart component it renders horizontal bar 1`] = ` position="bottom" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -407,7 +408,6 @@ exports[`xy_expression XYChart component it renders line 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -537,7 +538,6 @@ exports[`xy_expression XYChart component it renders stacked area 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -665,7 +666,6 @@ exports[`xy_expression XYChart component it renders stacked bar 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} - title="a" /> @@ -793,7 +794,6 @@ exports[`xy_expression XYChart component it renders stacked horizontal bar 1`] = position="bottom" showGridLines={false} tickFormat={[Function]} - title="a" /> legendConfig); expressions.registerFunction(() => yAxisConfig); + expressions.registerFunction(() => tickLabelsConfig); + expressions.registerFunction(() => gridlinesConfig); expressions.registerFunction(() => layerConfig); expressions.registerFunction(() => xyChart); 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 95676f3e59493..d156db2d61098 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 @@ -41,7 +41,8 @@ describe('#toExpression', () => { legend: { position: Position.Bottom, isVisible: true }, preferredSeriesType: 'bar', fittingFunction: 'Carry', - showXAxisGridlines: true, + tickLabelsVisibilitySettings: { x: false, y: true }, + gridlinesVisibilitySettings: { x: false, y: true }, layers: [ { layerId: 'first', @@ -78,7 +79,7 @@ describe('#toExpression', () => { ).toEqual('None'); }); - it('should default the hideXAxisTitle, showXAxisGridlines and hideXAxisTickLabels to false', () => { + it('should default the showXAxisTitle and showYAxisTitle to true', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, @@ -95,9 +96,8 @@ describe('#toExpression', () => { }, frame ) as Ast; - expect(expression.chain[0].arguments.hideXAxisTitle[0]).toBeFalsy(); - expect(expression.chain[0].arguments.showXAxisGridlines[0]).toBeFalsy(); - expect(expression.chain[0].arguments.hideXAxisTickLabels[0]).toBeFalsy(); + expect(expression.chain[0].arguments.showXAxisTitle[0]).toBe(true); + expect(expression.chain[0].arguments.showYAxisTitle[0]).toBe(true); }); it('should not generate an expression when missing x', () => { @@ -175,4 +175,54 @@ describe('#toExpression', () => { }), ]); }); + + it('should default the tick labels visibility settings to true', () => { + const expression = xyVisualization.toExpression( + { + legend: { position: Position.Bottom, isVisible: true }, + preferredSeriesType: 'bar', + layers: [ + { + layerId: 'first', + seriesType: 'area', + splitAccessor: 'd', + xAccessor: 'a', + accessors: ['b', 'c'], + }, + ], + }, + frame + ) as Ast; + expect( + (expression.chain[0].arguments.tickLabelsVisibilitySettings[0] as Ast).chain[0].arguments.x + ).toBeTruthy(); + expect( + (expression.chain[0].arguments.tickLabelsVisibilitySettings[0] as Ast).chain[0].arguments.y + ).toBeTruthy(); + }); + + it('should default the gridlines visibility settings to false', () => { + const expression = xyVisualization.toExpression( + { + legend: { position: Position.Bottom, isVisible: true }, + preferredSeriesType: 'bar', + layers: [ + { + layerId: 'first', + seriesType: 'area', + splitAccessor: 'd', + xAccessor: 'a', + accessors: ['b', 'c'], + }, + ], + }, + frame + ) as Ast; + expect( + (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments.x + ).toBeTruthy(); + expect( + (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments.y + ).toBeTruthy(); + }); }); 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 8c1ac19731606..54ea190447071 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -117,7 +117,7 @@ export const buildExpression = ( function: 'lens_xy_chart', arguments: { xTitle: [state.xTitle || xTitle], - yTitle: [yTitle], + yTitle: [state.yTitle || yTitle], legend: [ { type: 'expression', @@ -134,9 +134,46 @@ export const buildExpression = ( }, ], fittingFunction: [state.fittingFunction || 'None'], - hideXAxisTitle: [state.hideXAxisTitle || false], - showXAxisGridlines: [state.showXAxisGridlines || false], - hideXAxisTickLabels: [state.hideXAxisTickLabels || false], + showXAxisTitle: [state.showXAxisTitle ?? true], + showYAxisTitle: [state.showYAxisTitle ?? true], + tickLabelsVisibilitySettings: [ + { + type: 'expression', + chain: [ + { + type: 'function', + function: 'lens_xy_tickLabelsConfig', + arguments: { + x: [ + state?.tickLabelsVisibilitySettings + ? state.tickLabelsVisibilitySettings.x + : true, + ], + y: [ + state?.tickLabelsVisibilitySettings + ? state.tickLabelsVisibilitySettings.y + : true, + ], + }, + }, + ], + }, + ], + gridlinesVisibilitySettings: [ + { + type: 'expression', + chain: [ + { + type: 'function', + function: 'lens_xy_gridlinesConfig', + arguments: { + x: [state?.gridlinesVisibilitySettings?.x], + y: [state?.gridlinesVisibilitySettings?.y], + }, + }, + ], + }, + ], 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 7f85f1ce8dfa1..9972f1370061e 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -59,6 +59,81 @@ export const legendConfig: ExpressionFunctionDefinition< }, }; +export interface AxesSettingsConfig { + x: boolean; + y: boolean; +} + +type TickLabelsConfigResult = AxesSettingsConfig & { type: 'lens_xy_tickLabelsConfig' }; + +export const tickLabelsConfig: ExpressionFunctionDefinition< + 'lens_xy_tickLabelsConfig', + null, + AxesSettingsConfig, + TickLabelsConfigResult +> = { + name: 'lens_xy_tickLabelsConfig', + aliases: [], + type: 'lens_xy_tickLabelsConfig', + help: `Configure the xy chart's tick labels appearance`, + inputTypes: ['null'], + args: { + x: { + types: ['boolean'], + help: i18n.translate('xpack.lens.xyChart.tickLabels.help', { + defaultMessage: 'Specifies whether or not the tick labels are visible.', + }), + }, + y: { + types: ['boolean'], + help: i18n.translate('xpack.lens.xyChart.tickLabels.help', { + defaultMessage: 'Specifies whether or not the tick labels are visible.', + }), + }, + }, + fn: function fn(input: unknown, args: TickLabelsConfig) { + return { + type: 'lens_xy_tickLabelsConfig', + ...args, + }; + }, +}; + +type GridlinesConfigResult = AxesSettingsConfig & { type: 'lens_xy_gridlinesConfig' }; + +export const gridlinesConfig: ExpressionFunctionDefinition< + 'lens_xy_gridlinesConfig', + null, + AxesSettingsConfig, + GridlinesConfigResult +> = { + name: 'lens_xy_gridlinesConfig', + aliases: [], + type: 'lens_xy_gridlinesConfig', + help: `Configure the xy chart's gridlines appearance`, + inputTypes: ['null'], + args: { + x: { + types: ['boolean'], + help: i18n.translate('xpack.lens.xyChart.gridlines.help', { + defaultMessage: 'Specifies whether or not the gridlines are visible.', + }), + }, + y: { + types: ['boolean'], + help: i18n.translate('xpack.lens.xyChart.gridlines.help', { + defaultMessage: 'Specifies whether or not the gridlines are visible.', + }), + }, + }, + fn: function fn(input: unknown, args: gridlinesConfig) { + return { + type: 'lens_xy_gridlinesConfig', + ...args, + }; + }, +}; + interface AxisConfig { title: string; hide?: boolean; @@ -227,9 +302,10 @@ export interface XYArgs { legend: LegendConfig & { type: 'lens_xy_legendConfig' }; layers: LayerArgs[]; fittingFunction?: FittingFunction; - hideXAxisTitle?: boolean; - showXAxisGridlines?: boolean; - hideXAxisTickLabels?: boolean; + showXAxisTitle?: boolean; + showYAxisTitle?: boolean; + tickLabelsVisibilitySettings: AxesSettingsConfig & { type: 'lens_xy_tickLabelsConfig' }; + gridlinesVisibilitySettings: AxesSettingsConfig & { type: 'lens_xy_gridlinesConfig' }; } // Persisted parts of the state @@ -239,9 +315,10 @@ export interface XYState { fittingFunction?: FittingFunction; layers: LayerConfig[]; xTitle?: string; - hideXAxisTitle?: boolean; - showXAxisGridlines?: boolean; - hideXAxisTickLabels?: boolean; + showXAxisTitle?: boolean; + showYAxisTitle?: boolean; + tickLabelsVisibilitySettings: AxesSettingsConfig; + gridlinesVisibilitySettings: AxesSettingsConfig; } export type State = XYState; 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 770ad712cae21..9e4a79dca56a4 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 @@ -128,7 +128,7 @@ describe('XY Config panels', () => { expect(component.find(EuiSuperSelect).prop('disabled')).toEqual(true); }); - it('should show the hideXAxisTitle, hideXAxisTickLabels and showXAxisGridlines Switches on', () => { + it('should show the values of the X and Y axes titles on the corresponding input text', () => { const state = testState(); const component = shallow( @@ -137,25 +137,21 @@ describe('XY Config panels', () => { setState={jest.fn()} state={{ ...state, - hideXAxisTitle: true, - hideXAxisTickLabels: true, - showXAxisGridlines: true, + xTitle: 'My custom X axis title', + yTitle: 'My custom Y axis title', }} /> ); - expect( - component.find('[data-test-subj="lnsHideXAxisTitleSwitch"]').prop('checked') - ).toBeTruthy(); - expect( - component.find('[data-test-subj="lnsHideXAxisTickLabelsSwitch"]').prop('checked') - ).toBeTruthy(); - expect( - component.find('[data-test-subj="lnsShowXAxisGridLinesSwitch"]').prop('checked') - ).toBeTruthy(); + expect(component.find('[data-test-subj="lnsXAxisTitle"]').prop('value')).toBe( + 'My custom X axis title' + ); + expect(component.find('[data-test-subj="lnsYAxisTitle"]').prop('value')).toBe( + 'My custom Y axis title' + ); }); - it('should show the value of the X axis title on the corresponding input text', () => { + it('should disable the input texts if the switch is off', () => { const state = testState(); const component = shallow( @@ -164,14 +160,14 @@ describe('XY Config panels', () => { setState={jest.fn()} state={{ ...state, - xTitle: 'My custom X axis title', + showXAxisTitle: false, + showYAxisTitle: false, }} /> ); - expect(component.find('[data-test-subj="lnsXAxisTitle"]').prop('value')).toBe( - 'My custom X axis title' - ); + expect(component.find('[data-test-subj="lnsXAxisTitle"]').prop('disabled')).toBe(true); + expect(component.find('[data-test-subj="lnsYAxisTitle"]').prop('disabled')).toBe(true); }); }); }); 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 fefe308b6e8e1..3bbeb8df597aa 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 @@ -32,7 +32,7 @@ import { VisualizationDimensionEditorProps, VisualizationToolbarProps, } from '../types'; -import { State, SeriesType, visualizationTypes, YAxisMode } from './types'; +import { State, SeriesType, visualizationTypes, YAxisMode, AxesSettingsConfig } from './types'; import { isHorizontalChart, isHorizontalSeries, getSeriesColor } from './state_helpers'; import { trackUiEvent } from '../lens_ui_telemetry'; import { fittingFunctionDefinitions } from './fitting_functions'; @@ -95,17 +95,78 @@ export function LayerContextMenu(props: VisualizationLayerWidgetProps) { } export function XyToolbar(props: VisualizationToolbarProps) { + const axes = [ + { + id: 'x', + label: 'X-axis', + }, + { + id: 'y', + label: 'Y-axis', + }, + ]; + const [open, setOpen] = useState(false); const hasNonBarSeries = props.state?.layers.some( (layer) => layer.seriesType === 'line' || layer.seriesType === 'area' ); const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); + const [ytitle, setYtitle] = useState(props.state?.yTitle || ''); + const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState< + AxesSettingsConfig + >({ + ['x']: props.state?.tickLabelsVisibilitySettings + ? props.state.tickLabelsVisibilitySettings.x + : true, + ['y']: props.state?.tickLabelsVisibilitySettings + ? props.state.tickLabelsVisibilitySettings.y + : true, + }); + const [gridlinesVisibilitySettings, setGridlinesVisibilitySettings] = useState< + AxesSettingsConfig + >({ + ['x']: props.state?.gridlinesVisibilitySettings?.x, + ['y']: props.state?.gridlinesVisibilitySettings?.y, + }); const onXTitleChange = (value: string): void => { setXtitle(value); props.setState({ ...props.state, xTitle: value }); }; + const onYTitleChange = (value: string): void => { + setYtitle(value); + props.setState({ ...props.state, yTitle: value }); + }; + + const onTickLabelsVisibilitySettingsChange = (optionId) => { + const newTickLabelsVisibilitySettings = { + ...tickLabelsVisibilitySettings, + ...{ + [optionId]: !tickLabelsVisibilitySettings[optionId], + }, + }; + setTickLabelsVisibilitySettings(newTickLabelsVisibilitySettings); + props.setState({ + ...props.state, + tickLabelsVisibilitySettings: newTickLabelsVisibilitySettings, + }); + }; + + const onGridlinesVisibilitySettingsChange = (optionId) => { + const newGridlinesVisibilitySettings = { + ...gridlinesVisibilitySettings, + ...{ + [optionId]: !gridlinesVisibilitySettings[optionId], + }, + }; + setGridlinesVisibilitySettings(newGridlinesVisibilitySettings); + props.setState({ + ...props.state, + gridlinesVisibilitySettings: newGridlinesVisibilitySettings, + }); + }; + return ( @@ -169,79 +230,110 @@ export function XyToolbar(props: VisualizationToolbarProps) { - - X Axis - - onXTitleChange(target.value)} - aria-label="Overwrite X Axis Title" + onTickLabelsVisibilitySettingsChange(id)} + buttonSize="compressed" + isFullWidth + type="multi" /> - - props.setState({ ...props.state, hideXAxisTitle: target.checked }) - } - checked={props.state?.hideXAxisTitle || false} + onGridlinesVisibilitySettingsChange(id)} + buttonSize="compressed" + isFullWidth + type="multi" /> + + + Axis Titles + + X-axis + + + props.setState({ ...props.state, showXAxisTitle: target.checked }) + } + checked={props.state?.showXAxisTitle ?? true} + /> + + + } > - - props.setState({ ...props.state, hideXAxisTickLabels: target.checked }) + onXTitleChange(target.value)} + aria-label="Overwrite X-axis title" /> + Y-axis + + + props.setState({ ...props.state, showYAxisTitle: target.checked }) + } + checked={props.state?.showYAxisTitle ?? true} + /> + + + } > - - props.setState({ ...props.state, showXAxisGridlines: target.checked }) + onYTitleChange(target.value)} + aria-label="Overwrite Y-axis title" /> - diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx index 18294e2575d05..1e1a0b3c263b8 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.test.tsx @@ -22,7 +22,15 @@ import { LensMultiTable } from '../types'; import { KibanaDatatable, KibanaDatatableRow } from '../../../../../src/plugins/expressions/public'; import React from 'react'; import { shallow } from 'enzyme'; -import { XYArgs, LegendConfig, legendConfig, layerConfig, LayerArgs } from './types'; +import { + XYArgs, + LegendConfig, + legendConfig, + layerConfig, + LayerArgs, + AxesSettingsConfig, + tickLabelsConfig, +} from './types'; import { createMockExecutionContext } from '../../../../../src/plugins/expressions/common/mocks'; import { mountWithIntl } from 'test_utils/enzyme_helpers'; import { chartPluginMock } from '../../../../../src/plugins/charts/public/mocks'; @@ -211,6 +219,18 @@ const createArgsWithLayers = (layers: LayerArgs[] = [sampleLayer]): XYArgs => ({ isVisible: false, position: Position.Top, }, + showXAxisTitle: true, + showyAxisTitle: true, + tickLabelsVisibilitySettings: { + type: 'lens_xy_tickLabelsConfig', + x: true, + y: false, + }, + gridlinesVisibilitySettings: { + type: 'lens_xy_gridlinesConfig', + x: true, + y: false, + }, layers, }); @@ -267,6 +287,20 @@ describe('xy_expression', () => { }); }); + test('tickLabelsConfig produces the correct arguments', () => { + const args: AxesSettingsConfig = { + x: true, + y: false, + }; + + const result = tickLabelsConfig.fn(null, args, createMockExecutionContext()); + + expect(result).toEqual({ + type: 'lens_xy_tickLabelsConfig', + ...args, + }); + }); + describe('xyChart', () => { test('it renders with the specified data and args', () => { const { data, args } = sampleArgs(); @@ -1365,10 +1399,10 @@ describe('xy_expression', () => { expect(convertSpy).toHaveBeenCalledWith('I'); }); - test('it should not pass the formatter function to the axis if the hideXAxisTickLabels switch is on', () => { + test('it should not pass the formatter function to the x axis if the visibility of the tick labels is off', () => { const { data, args } = sampleArgs(); - args.hideXAxisTickLabels = true; + args.tickLabelsVisibilitySettings.x = false; const instance = shallow( { xTitle: '', yTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, + tickLabelsVisibilitySettings: { + type: 'lens_xy_tickLabelsConfig', + x: true, + y: true, + }, + gridlinesVisibilitySettings: { + type: 'lens_xy_gridlinesConfig', + x: true, + y: false, + }, layers: [ { layerId: 'first', @@ -1498,6 +1542,16 @@ describe('xy_expression', () => { xTitle: '', yTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top }, + tickLabelsVisibilitySettings: { + type: 'lens_xy_tickLabelsConfig', + x: true, + y: false, + }, + gridlinesVisibilitySettings: { + type: 'lens_xy_gridlinesConfig', + x: true, + y: false, + }, layers: [ { layerId: 'first', @@ -1554,6 +1608,16 @@ describe('xy_expression', () => { xTitle: '', yTitle: '', legend: { type: 'lens_xy_legendConfig', isVisible: true, position: Position.Top }, + tickLabelsVisibilitySettings: { + type: 'lens_xy_tickLabelsConfig', + x: true, + y: false, + }, + gridlinesVisibilitySettings: { + type: 'lens_xy_gridlinesConfig', + x: true, + y: false, + }, layers: [ { layerId: 'first', @@ -1670,7 +1734,7 @@ describe('xy_expression', () => { test('it should hide the X axis title if the corresponding switch is on', () => { const { data, args } = sampleArgs(); - args.hideXAxisTitle = true; + args.showXAxisTitle = false; const component = shallow( xAxisFormatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings.x ? (d) => xAxisFormatter.convert(d) : () => {}} /> {yAxesConfiguration.map((axis, index) => ( @@ -407,17 +412,22 @@ export function XYChart({ groupId={axis.groupId} position={axis.position} title={ - axis.series - .map( - (series) => - data.tables[series.layer].columns.find((column) => column.id === series.accessor) - ?.name - ) - .filter((name) => Boolean(name))[0] || args.yTitle + showYAxisTitle + ? args.yTitle || + axis.series + .map( + (series) => + data.tables[series.layer].columns.find( + (column) => column.id === series.accessor + )?.name + ) + .filter((name) => Boolean(name))[0] + : undefined } - showGridLines={false} + showGridLines={gridlinesVisibilitySettings.y} hide={filteredLayers[0].hide} - tickFormat={(d) => axis.formatter.convert(d)} + // @ts-ignore, temporary solution for not displaying the ticks + tickFormat={tickLabelsVisibilitySettings.y ? (d) => xAxisFormatter.convert(d) : () => {}} /> ))} 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 aae17a2f47f43..e740cc9ad7974 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 @@ -332,9 +332,10 @@ describe('xy_suggestions', () => { const currentState: XYState = { legend: { isVisible: true, position: 'bottom' }, fittingFunction: 'None', - hideXAxisTitle: false, - showXAxisGridlines: false, - hideXAxisTickLabels: false, + showXAxisTitle: true, + showYAxisTitle: true, + gridlinesVisibilitySettings: { x: true, y: true }, + tickLabelsVisibilitySettings: { x: true, y: false }, preferredSeriesType: 'bar', layers: [ { @@ -373,9 +374,10 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', - hideXAxisTitle: false, - showXAxisGridlines: false, - hideXAxisTickLabels: false, + showXAxisTitle: true, + showYAxisTitle: true, + gridlinesVisibilitySettings: { x: true, y: true }, + tickLabelsVisibilitySettings: { x: true, y: false }, layers: [ { accessors: ['price', 'quantity'], @@ -485,9 +487,10 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', - hideXAxisTitle: false, - showXAxisGridlines: false, - hideXAxisTickLabels: false, + showXAxisTitle: true, + showYAxisTitle: true, + gridlinesVisibilitySettings: { x: true, y: true }, + tickLabelsVisibilitySettings: { x: true, y: false }, layers: [ { accessors: ['price', 'quantity'], @@ -527,9 +530,10 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', - hideXAxisTitle: false, - showXAxisGridlines: false, - hideXAxisTickLabels: false, + showXAxisTitle: true, + showYAxisTitle: true, + gridlinesVisibilitySettings: { x: true, y: true }, + tickLabelsVisibilitySettings: { x: true, y: false }, layers: [ { accessors: ['price'], @@ -570,9 +574,10 @@ describe('xy_suggestions', () => { legend: { isVisible: true, position: 'bottom' }, preferredSeriesType: 'bar', fittingFunction: 'None', - hideXAxisTitle: false, - showXAxisGridlines: false, - hideXAxisTickLabels: false, + showXAxisTitle: true, + showYAxisTitle: true, + gridlinesVisibilitySettings: { x: true, y: true }, + tickLabelsVisibilitySettings: { x: true, y: false }, layers: [ { accessors: ['price', 'quantity'], 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 6b52cab12d1c4..e9ece210cd016 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts @@ -404,9 +404,17 @@ function buildSuggestion({ legend: currentState ? currentState.legend : { isVisible: true, position: Position.Right }, fittingFunction: currentState?.fittingFunction || 'None', xTitle: currentState?.xTitle, - hideXAxisTitle: currentState?.hideXAxisTitle || false, - showXAxisGridlines: currentState?.showXAxisGridlines || false, - hideXAxisTickLabels: currentState?.hideXAxisTickLabels || false, + yTitle: currentState?.yTitle, + showXAxisTitle: currentState?.showXAxisTitle ?? true, + showYAxisTitle: currentState?.showYAxisTitle ?? true, + tickLabelsVisibilitySettings: currentState?.tickLabelsVisibilitySettings || { + x: true, + y: true, + }, + gridlinesVisibilitySettings: currentState?.gridlinesVisibilitySettings || { + x: false, + y: false, + }, preferredSeriesType: seriesType, layers: [...keptLayers, newLayer], }; From a5c7aae239c2a943b17e4c1106f498b25f087041 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Fri, 17 Jul 2020 13:27:13 +0300 Subject: [PATCH 04/14] fix types and add unit tests --- .../__snapshots__/xy_expression.test.tsx.snap | 7 ++++ .../public/xy_visualization/to_expression.ts | 16 +++------ .../lens/public/xy_visualization/types.ts | 13 +++---- .../xy_visualization/xy_config_panel.test.tsx | 30 ++++++++++++++++ .../xy_visualization/xy_config_panel.tsx | 36 +++++++++---------- .../xy_visualization/xy_expression.test.tsx | 25 ++++++++++--- .../public/xy_visualization/xy_expression.tsx | 4 +-- 7 files changed, 87 insertions(+), 44 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap index 61a90fc88fa26..f0c233b44a285 100644 --- a/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap +++ b/x-pack/plugins/lens/public/xy_visualization/__snapshots__/xy_expression.test.tsx.snap @@ -38,6 +38,7 @@ exports[`xy_expression XYChart component it renders area 1`] = ` position="left" showGridLines={false} tickFormat={[Function]} + title="a" /> { expect(component.find('[data-test-subj="lnsXAxisTitle"]').prop('disabled')).toBe(true); expect(component.find('[data-test-subj="lnsYAxisTitle"]').prop('disabled')).toBe(true); }); + + it('has the tick labels buttons enabled', () => { + const state = testState(); + + const component = shallow(); + + const options = component + .find('[data-test-subj="lnsTickLabelsSettings"]') + .prop('options') as EuiButtonGroupProps['options']; + + expect(options!.map(({ label }) => label)).toEqual(['X-axis', 'Y-axis']); + + const selections = component + .find('[data-test-subj="lnsTickLabelsSettings"]') + .prop('idToSelectedMap'); + + expect(selections!).toEqual({ x: true, y: true }); + }); + + it('has the gridlines buttons disabled', () => { + const state = testState(); + + const component = shallow(); + + const selections = component + .find('[data-test-subj="lnsGridlinesSettings"]') + .prop('idToSelectedMap'); + + expect(selections!).toEqual({ x: false, y: false }); + }); }); }); 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 3bbeb8df597aa..641b758eefbb4 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 @@ -112,21 +112,13 @@ export function XyToolbar(props: VisualizationToolbarProps) { ); const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); const [ytitle, setYtitle] = useState(props.state?.yTitle || ''); - const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState< - AxesSettingsConfig - >({ - ['x']: props.state?.tickLabelsVisibilitySettings - ? props.state.tickLabelsVisibilitySettings.x - : true, - ['y']: props.state?.tickLabelsVisibilitySettings - ? props.state.tickLabelsVisibilitySettings.y - : true, + const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState({ + x: props.state?.tickLabelsVisibilitySettings?.x ?? true, + y: props.state?.tickLabelsVisibilitySettings?.y ?? true, }); - const [gridlinesVisibilitySettings, setGridlinesVisibilitySettings] = useState< - AxesSettingsConfig - >({ - ['x']: props.state?.gridlinesVisibilitySettings?.x, - ['y']: props.state?.gridlinesVisibilitySettings?.y, + const [gridlinesVisibilitySettings, setGridlinesVisibilitySettings] = useState({ + x: props.state?.gridlinesVisibilitySettings?.x || false, + y: props.state?.gridlinesVisibilitySettings?.y || false, }); const onXTitleChange = (value: string): void => { @@ -139,11 +131,14 @@ export function XyToolbar(props: VisualizationToolbarProps) { props.setState({ ...props.state, yTitle: value }); }; - const onTickLabelsVisibilitySettingsChange = (optionId) => { + type AxesSettingsConfigKeys = keyof AxesSettingsConfig; + + const onTickLabelsVisibilitySettingsChange = (optionId: string): void => { + const id = optionId as AxesSettingsConfigKeys; const newTickLabelsVisibilitySettings = { ...tickLabelsVisibilitySettings, ...{ - [optionId]: !tickLabelsVisibilitySettings[optionId], + [id]: !tickLabelsVisibilitySettings[id], }, }; setTickLabelsVisibilitySettings(newTickLabelsVisibilitySettings); @@ -153,11 +148,12 @@ export function XyToolbar(props: VisualizationToolbarProps) { }); }; - const onGridlinesVisibilitySettingsChange = (optionId) => { + const onGridlinesVisibilitySettingsChange = (optionId: string): void => { + const id = optionId as AxesSettingsConfigKeys; const newGridlinesVisibilitySettings = { ...gridlinesVisibilitySettings, ...{ - [optionId]: !gridlinesVisibilitySettings[optionId], + [id]: !gridlinesVisibilitySettings[id], }, }; setGridlinesVisibilitySettings(newGridlinesVisibilitySettings); @@ -238,6 +234,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { > ) { })} > ({ position: Position.Top, }, showXAxisTitle: true, - showyAxisTitle: true, + showYAxisTitle: true, tickLabelsVisibilitySettings: { type: 'lens_xy_tickLabelsConfig', x: true, @@ -301,6 +302,20 @@ describe('xy_expression', () => { }); }); + test('gridlinesConfig produces the correct arguments', () => { + const args: AxesSettingsConfig = { + x: true, + y: false, + }; + + const result = gridlinesConfig.fn(null, args, createMockExecutionContext()); + + expect(result).toEqual({ + type: 'lens_xy_gridlinesConfig', + ...args, + }); + }); + describe('xyChart', () => { test('it renders with the specified data and args', () => { const { data, args } = sampleArgs(); @@ -1402,7 +1417,7 @@ describe('xy_expression', () => { test('it should not pass the formatter function to the x axis if the visibility of the tick labels is off', () => { const { data, args } = sampleArgs(); - args.tickLabelsVisibilitySettings.x = false; + args.tickLabelsVisibilitySettings = { x: false, y: true, type: 'lens_xy_tickLabelsConfig' }; const instance = shallow( { expect(component.find(Axis).at(0).prop('title')).toEqual('My custom x-axis title'); }); - test('it should hide the X axis title if the corresponding switch is on', () => { + test('it should hide the X axis title if the corresponding switch is off', () => { const { data, args } = sampleArgs(); args.showXAxisTitle = false; @@ -1752,10 +1767,10 @@ describe('xy_expression', () => { expect(component.find(Axis).at(0).prop('title')).toEqual(undefined); }); - test('it should show the X axis gridlines if the corresponding switch is on', () => { + test('it should show the X axis gridlines if the setting is on', () => { const { data, args } = sampleArgs(); - args.showXAxisGridlines = true; + args.gridlinesVisibilitySettings = { x: true, y: false, type: 'lens_xy_gridlinesConfig' }; const component = shallow( Boolean(name))[0] : undefined } - showGridLines={gridlinesVisibilitySettings.y} + showGridLines={gridlinesVisibilitySettings?.y} hide={filteredLayers[0].hide} // @ts-ignore, temporary solution for not displaying the ticks tickFormat={tickLabelsVisibilitySettings.y ? (d) => xAxisFormatter.convert(d) : () => {}} From 036ef3a65dfd940edc1382abaaf88937d8239aa1 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Fri, 17 Jul 2020 13:50:00 +0300 Subject: [PATCH 05/14] Add extra translations --- .../lens/public/xy_visualization/types.ts | 16 +++++++-------- .../xy_visualization/xy_config_panel.tsx | 20 ++++++++++++++----- .../public/xy_visualization/xy_expression.tsx | 16 +++++++++++---- 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/types.ts b/x-pack/plugins/lens/public/xy_visualization/types.ts index 1bb1bbe598626..c2f3cf42b196c 100644 --- a/x-pack/plugins/lens/public/xy_visualization/types.ts +++ b/x-pack/plugins/lens/public/xy_visualization/types.ts @@ -80,14 +80,14 @@ export const tickLabelsConfig: ExpressionFunctionDefinition< args: { x: { types: ['boolean'], - help: i18n.translate('xpack.lens.xyChart.tickLabels.help', { - defaultMessage: 'Specifies whether or not the tick labels are visible.', + help: i18n.translate('xpack.lens.xyChart.xAxisTickLabels.help', { + defaultMessage: 'Specifies whether or not the tick labels of the x-axis are visible.', }), }, y: { types: ['boolean'], - help: i18n.translate('xpack.lens.xyChart.tickLabels.help', { - defaultMessage: 'Specifies whether or not the tick labels are visible.', + help: i18n.translate('xpack.lens.xyChart.yAxisTickLabels.help', { + defaultMessage: 'Specifies whether or not the tick labels of the y-axis are visible.', }), }, }, @@ -115,14 +115,14 @@ export const gridlinesConfig: ExpressionFunctionDefinition< args: { x: { types: ['boolean'], - help: i18n.translate('xpack.lens.xyChart.gridlines.help', { - defaultMessage: 'Specifies whether or not the gridlines are visible.', + help: i18n.translate('xpack.lens.xyChart.xAxisGridlines.help', { + defaultMessage: 'Specifies whether or not the gridlines of the x-axis are visible.', }), }, y: { types: ['boolean'], - help: i18n.translate('xpack.lens.xyChart.gridlines.help', { - defaultMessage: 'Specifies whether or not the gridlines are visible.', + help: i18n.translate('xpack.lens.xyChart.yAxisgridlines.help', { + defaultMessage: 'Specifies whether or not the gridlines of the y-axis are visible.', }), }, }, 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 641b758eefbb4..58b0e4a04de55 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 @@ -264,7 +264,9 @@ export function XyToolbar(props: VisualizationToolbarProps) { - Axis Titles + + {i18n.translate('xpack.lens.xyChart.axisTitles', { defaultMessage: 'Axis titles' })} + ) { onXTitleChange(target.value)} - aria-label="Overwrite X-axis title" + aria-label={i18n.translate('xpack.lens.xyChart.overwriteXaxis', { + defaultMessage: 'Overwrite X-axis title', + })} /> ) { onYTitleChange(target.value)} - aria-label="Overwrite Y-axis title" + aria-label={i18n.translate('xpack.lens.xyChart.overwriteYaxis', { + defaultMessage: 'Overwrite Y-axis title', + })} /> diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 0a16092599535..4e7d0a0c928ed 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -104,19 +104,27 @@ export const xyChart: ExpressionFunctionDefinition< }, tickLabelsVisibilitySettings: { types: ['lens_xy_tickLabelsConfig'], - help: 'Show x and y axes tick labels', + help: i18n.translate('xpack.lens.xyChart.tickLabelsSettings.help', { + defaultMessage: 'Show x and y axes tick labels', + }), }, gridlinesVisibilitySettings: { types: ['lens_xy_gridlinesConfig'], - help: 'Show x and y gridlines', + help: i18n.translate('xpack.lens.xyChart.gridlinesSettings.help', { + defaultMessage: 'Show x and y axes gridlines', + }), }, showXAxisTitle: { types: ['boolean'], - help: 'Hide X-axis title', + help: i18n.translate('xpack.lens.xyChart.showXAxisTitle.help', { + defaultMessage: 'Show x axis title', + }), }, showYAxisTitle: { types: ['boolean'], - help: 'Hide Y-axis title', + help: i18n.translate('xpack.lens.xyChart.showYAxisTitle.help', { + defaultMessage: 'Show y axis title', + }), }, layers: { // eslint-disable-next-line @typescript-eslint/no-explicit-any From 96d60b7fe6d829c3de241c1da3413e8651d67173 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Fri, 17 Jul 2020 15:57:44 +0300 Subject: [PATCH 06/14] Fix functional test and change the logic of the yTitle --- .../__snapshots__/to_expression.test.ts.snap | 4 +-- .../xy_visualization/to_expression.test.ts | 4 +-- .../public/xy_visualization/to_expression.ts | 31 +++---------------- .../public/xy_visualization/xy_expression.tsx | 31 ++++++++++--------- 4 files changed, 24 insertions(+), 46 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 10d17572c8962..bd8ce58a886ca 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 @@ -117,10 +117,10 @@ Object { }, ], "xTitle": Array [ - "col_a", + "", ], "yTitle": Array [ - "col_b", + "", ], }, "function": "lens_xy_chart", 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 d156db2d61098..19f3c4bac1f93 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 @@ -163,8 +163,8 @@ describe('#toExpression', () => { expect(mockDatasource.publicAPIMock.getOperationForColumnId).toHaveBeenCalledWith('b'); expect(mockDatasource.publicAPIMock.getOperationForColumnId).toHaveBeenCalledWith('c'); expect(mockDatasource.publicAPIMock.getOperationForColumnId).toHaveBeenCalledWith('d'); - expect(expression.chain[0].arguments.xTitle).toEqual(['col_a']); - expect(expression.chain[0].arguments.yTitle).toEqual(['col_b']); + expect(expression.chain[0].arguments.xTitle).toEqual(['']); + expect(expression.chain[0].arguments.yTitle).toEqual(['']); expect( (expression.chain[0].arguments.layers[0] as Ast).chain[0].arguments.columnToLabel ).toEqual([ 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 8abbfe2434608..c1e147ec5db1c 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -13,28 +13,6 @@ interface ValidLayer extends LayerConfig { xAccessor: NonNullable; } -function xyTitles(layer: LayerConfig, frame: FramePublicAPI) { - const defaults = { - xTitle: 'x', - yTitle: 'y', - }; - - if (!layer || !layer.accessors.length) { - return defaults; - } - const datasource = frame.datasourceLayers[layer.layerId]; - if (!datasource) { - return defaults; - } - const x = layer.xAccessor ? datasource.getOperationForColumnId(layer.xAccessor) : null; - const y = layer.accessors[0] ? datasource.getOperationForColumnId(layer.accessors[0]) : null; - - return { - xTitle: x ? x.label : defaults.xTitle, - yTitle: y ? y.label : defaults.yTitle, - }; -} - export const toExpression = (state: State, frame: FramePublicAPI): Ast | null => { if (!state || !state.layers.length) { return null; @@ -52,7 +30,7 @@ export const toExpression = (state: State, frame: FramePublicAPI): Ast | null => }); }); - return buildExpression(state, metadata, frame, xyTitles(state.layers[0], frame)); + return buildExpression(state, metadata, frame); }; export function toPreviewExpression(state: State, frame: FramePublicAPI) { @@ -99,8 +77,7 @@ export function getScaleType(metadata: OperationMetadata | null, defaultScale: S export const buildExpression = ( state: State, metadata: Record>, - frame?: FramePublicAPI, - { xTitle, yTitle }: { xTitle: string; yTitle: string } = { xTitle: '', yTitle: '' } + frame?: FramePublicAPI ): Ast | null => { const validLayers = state.layers.filter((layer): layer is ValidLayer => Boolean(layer.xAccessor && layer.accessors.length) @@ -116,8 +93,8 @@ export const buildExpression = ( type: 'function', function: 'lens_xy_chart', arguments: { - xTitle: [state.xTitle || xTitle], - yTitle: [state.yTitle || yTitle], + xTitle: [state.xTitle || ''], + yTitle: [state.yTitle || ''], legend: [ { type: 'expression', diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 4e7d0a0c928ed..65146b3524456 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -311,6 +311,19 @@ export function XYChart({ } : undefined; + const getYAxesTitles = (axisSeries, index) => { + if (index > 0 && args.yTitle) return; + return ( + args.yTitle || + axisSeries + .map( + (series) => + data.tables[series.layer].columns.find((column) => column.id === series.accessor)?.name + ) + .filter((name) => Boolean(name))[0] + ); + }; + return ( xAxisFormatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings?.x ? (d) => xAxisFormatter.convert(d) : () => {}} /> {yAxesConfiguration.map((axis, index) => ( @@ -419,23 +432,11 @@ export function XYChart({ id={axis.groupId} groupId={axis.groupId} position={axis.position} - title={ - showYAxisTitle - ? args.yTitle || - axis.series - .map( - (series) => - data.tables[series.layer].columns.find( - (column) => column.id === series.accessor - )?.name - ) - .filter((name) => Boolean(name))[0] - : undefined - } + title={showYAxisTitle ? getYAxesTitles(axis.series, index) : undefined} showGridLines={gridlinesVisibilitySettings?.y} hide={filteredLayers[0].hide} // @ts-ignore, temporary solution for not displaying the ticks - tickFormat={tickLabelsVisibilitySettings.y ? (d) => xAxisFormatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings?.y ? (d) => xAxisFormatter.convert(d) : () => {}} /> ))} From ff14e6d6786047e1ddb74cb89d52269d257d526f Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Fri, 17 Jul 2020 17:36:57 +0300 Subject: [PATCH 07/14] fixes --- .../public/xy_visualization/xy_expression.tsx | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 65146b3524456..3eb690e832671 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -223,15 +223,7 @@ export function XYChart({ onClickValue, onSelectRange, }: XYChartRenderProps) { - const { - legend, - layers, - fittingFunction, - tickLabelsVisibilitySettings, - gridlinesVisibilitySettings, - showXAxisTitle, - showYAxisTitle, - } = args; + const { legend, layers, fittingFunction, gridlinesVisibilitySettings } = args; const chartTheme = chartsThemeService.useChartsTheme(); const chartBaseTheme = chartsThemeService.useChartsBaseTheme(); @@ -270,6 +262,9 @@ export function XYChart({ ); const xTitle = args.xTitle || (xAxisColumn && xAxisColumn.name); + const showXAxisTitle = args.showXAxisTitle || true; + const showYAxisTitle = args.showYAxisTitle || true; + const tickLabelsVisibilitySettings = args.tickLabelsVisibilitySettings || { x: true, y: true }; function calculateMinInterval() { // check all the tables to see if all of the rows have the same timestamp @@ -311,7 +306,10 @@ export function XYChart({ } : undefined; - const getYAxesTitles = (axisSeries, index) => { + const getYAxesTitles = ( + axisSeries: Array<{ layer: string; accessor: string }>, + index: number + ) => { if (index > 0 && args.yTitle) return; return ( args.yTitle || From 3ddb22f7552bcd7c4b113ce998bfa8f14db84ff7 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Fri, 17 Jul 2020 17:54:50 +0300 Subject: [PATCH 08/14] fix showTitle settings bug --- x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 3eb690e832671..5621aeca7ceae 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -262,8 +262,8 @@ export function XYChart({ ); const xTitle = args.xTitle || (xAxisColumn && xAxisColumn.name); - const showXAxisTitle = args.showXAxisTitle || true; - const showYAxisTitle = args.showYAxisTitle || true; + const showXAxisTitle = args.showXAxisTitle ?? true; + const showYAxisTitle = args.showYAxisTitle ?? true; const tickLabelsVisibilitySettings = args.tickLabelsVisibilitySettings || { x: true, y: true }; function calculateMinInterval() { From 56e0df12cd929b90f2e73509072b46c82d49260b Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Mon, 20 Jul 2020 10:38:40 +0300 Subject: [PATCH 09/14] Fix ticklabels bug on y axes --- x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 5621aeca7ceae..4a5f9809287de 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -434,7 +434,7 @@ export function XYChart({ showGridLines={gridlinesVisibilitySettings?.y} hide={filteredLayers[0].hide} // @ts-ignore, temporary solution for not displaying the ticks - tickFormat={tickLabelsVisibilitySettings?.y ? (d) => xAxisFormatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings?.y ? (d) => axis.formatter.convert(d) : () => {}} /> ))} From 8cb771bde3cf727a43c14c228ac6be3a9c855cb7 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Tue, 21 Jul 2020 13:32:00 +0300 Subject: [PATCH 10/14] fix some tests --- .../xy_visualization/to_expression.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 19f3c4bac1f93..79cdb847a2d01 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 @@ -194,11 +194,11 @@ describe('#toExpression', () => { frame ) as Ast; expect( - (expression.chain[0].arguments.tickLabelsVisibilitySettings[0] as Ast).chain[0].arguments.x - ).toBeTruthy(); - expect( - (expression.chain[0].arguments.tickLabelsVisibilitySettings[0] as Ast).chain[0].arguments.y - ).toBeTruthy(); + (expression.chain[0].arguments.tickLabelsVisibilitySettings[0] as Ast).chain[0].arguments + ).toEqual({ + x: [true], + y: [true], + }); }); it('should default the gridlines visibility settings to false', () => { @@ -219,10 +219,10 @@ describe('#toExpression', () => { frame ) as Ast; expect( - (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments.x - ).toBeTruthy(); - expect( - (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments.y - ).toBeTruthy(); + (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments + ).toEqual({ + x: [false], + y: [false], + }); }); }); From ab6ef2b9214cd40a5772eade918a0869d9980026 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Wed, 22 Jul 2020 12:18:26 +0300 Subject: [PATCH 11/14] Change the user flow on x and y titles on settings popover and enable the gridlines by default --- .../xy_visualization/to_expression.test.ts | 6 +- .../public/xy_visualization/to_expression.ts | 4 +- .../xy_visualization/xy_config_panel.test.tsx | 9 +- .../xy_visualization/xy_config_panel.tsx | 97 +++++++++++++------ .../public/xy_visualization/xy_suggestions.ts | 4 +- 5 files changed, 74 insertions(+), 46 deletions(-) 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 79cdb847a2d01..876d1141740e1 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 @@ -201,7 +201,7 @@ describe('#toExpression', () => { }); }); - it('should default the gridlines visibility settings to false', () => { + it('should default the gridlines visibility settings to true', () => { const expression = xyVisualization.toExpression( { legend: { position: Position.Bottom, isVisible: true }, @@ -221,8 +221,8 @@ describe('#toExpression', () => { expect( (expression.chain[0].arguments.gridlinesVisibilitySettings[0] as Ast).chain[0].arguments ).toEqual({ - x: [false], - y: [false], + x: [true], + y: [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 c1e147ec5db1c..798b7e81a62cd 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -136,8 +136,8 @@ export const buildExpression = ( type: 'function', function: 'lens_xy_gridlinesConfig', arguments: { - x: [state?.gridlinesVisibilitySettings?.x || false], - y: [state?.gridlinesVisibilitySettings?.y || false], + x: [state?.gridlinesVisibilitySettings?.x ?? true], + y: [state?.gridlinesVisibilitySettings?.y ?? 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 879e401eb2b5c..614b31e11d864 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 @@ -109,7 +109,6 @@ describe('XY Config panels', () => { it('should disable the select if there is no unstacked area or line series', () => { const state = testState(); - const component = shallow( { it('should show the values of the X and Y axes titles on the corresponding input text', () => { const state = testState(); - const component = shallow( { it('should disable the input texts if the switch is off', () => { const state = testState(); - const component = shallow( { it('has the tick labels buttons enabled', () => { const state = testState(); - const component = shallow(); const options = component @@ -188,16 +184,15 @@ describe('XY Config panels', () => { expect(selections!).toEqual({ x: true, y: true }); }); - it('has the gridlines buttons disabled', () => { + it('has the gridlines buttons enabled', () => { const state = testState(); - const component = shallow(); const selections = component .find('[data-test-subj="lnsGridlinesSettings"]') .prop('idToSelectedMap'); - expect(selections!).toEqual({ x: false, y: false }); + expect(selections!).toEqual({ x: true, y: true }); }); }); }); 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 58b0e4a04de55..ee4bf4d0163f7 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 @@ -5,7 +5,7 @@ */ import './xy_config_panel.scss'; -import React, { useState } from 'react'; +import React, { useState, useEffect, useCallback } from 'react'; import { i18n } from '@kbn/i18n'; import { debounce } from 'lodash'; import { @@ -106,29 +106,64 @@ export function XyToolbar(props: VisualizationToolbarProps) { }, ]; + const { frame, state, setState } = props; + const [open, setOpen] = useState(false); - const hasNonBarSeries = props.state?.layers.some( + const hasNonBarSeries = state?.layers.some( (layer) => layer.seriesType === 'line' || layer.seriesType === 'area' ); - const [xtitle, setXtitle] = useState(props.state?.xTitle || ''); - const [ytitle, setYtitle] = useState(props.state?.yTitle || ''); + + const [xaxistitle, setXaxistitle] = useState(state?.xTitle); + const [yaxistitle, setYaxistitle] = useState(state?.yTitle); + + const xyTitles = useCallback(() => { + const defaults = { + xTitle: state?.xTitle, + yTitle: state?.yTitle, + }; + const layer = state?.layers[0]; + if (!layer || !layer.accessors.length) { + return defaults; + } + const datasource = frame.datasourceLayers[layer.layerId]; + if (!datasource) { + return defaults; + } + const x = layer.xAccessor ? datasource.getOperationForColumnId(layer.xAccessor) : null; + const y = layer.accessors[0] ? datasource.getOperationForColumnId(layer.accessors[0]) : null; + + return { + xTitle: defaults.xTitle || x?.label, + yTitle: defaults.yTitle || y?.label, + }; + }, [open]); + + useEffect(() => { + const { + xTitle, + yTitle, + }: { xTitle: string | undefined; yTitle: string | undefined } = xyTitles(); + setXaxistitle(xTitle); + setYaxistitle(yTitle); + }, [xyTitles]); + const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState({ - x: props.state?.tickLabelsVisibilitySettings?.x ?? true, - y: props.state?.tickLabelsVisibilitySettings?.y ?? true, + x: state?.tickLabelsVisibilitySettings?.x ?? true, + y: state?.tickLabelsVisibilitySettings?.y ?? true, }); const [gridlinesVisibilitySettings, setGridlinesVisibilitySettings] = useState({ - x: props.state?.gridlinesVisibilitySettings?.x || false, - y: props.state?.gridlinesVisibilitySettings?.y || false, + x: state?.gridlinesVisibilitySettings?.x ?? true, + y: state?.gridlinesVisibilitySettings?.y ?? true, }); const onXTitleChange = (value: string): void => { - setXtitle(value); - props.setState({ ...props.state, xTitle: value }); + setXaxistitle(value); + setState({ ...state, xTitle: value }); }; const onYTitleChange = (value: string): void => { - setYtitle(value); - props.setState({ ...props.state, yTitle: value }); + setYaxistitle(value); + setState({ ...state, yTitle: value }); }; type AxesSettingsConfigKeys = keyof AxesSettingsConfig; @@ -142,8 +177,8 @@ export function XyToolbar(props: VisualizationToolbarProps) { }, }; setTickLabelsVisibilitySettings(newTickLabelsVisibilitySettings); - props.setState({ - ...props.state, + setState({ + ...state, tickLabelsVisibilitySettings: newTickLabelsVisibilitySettings, }); }; @@ -157,8 +192,8 @@ export function XyToolbar(props: VisualizationToolbarProps) { }, }; setGridlinesVisibilitySettings(newGridlinesVisibilitySettings); - props.setState({ - ...props.state, + setState({ + ...state, gridlinesVisibilitySettings: newGridlinesVisibilitySettings, }); }; @@ -218,8 +253,8 @@ export function XyToolbar(props: VisualizationToolbarProps) { inputDisplay: title, }; })} - valueOfSelected={props.state?.fittingFunction || 'None'} - onChange={(value) => props.setState({ ...props.state, fittingFunction: value })} + valueOfSelected={state?.fittingFunction || 'None'} + onChange={(value) => setState({ ...state, fittingFunction: value })} itemLayoutAlign="top" hasDividers /> @@ -272,18 +307,19 @@ export function XyToolbar(props: VisualizationToolbarProps) { display="columnCompressed" label={ - X-axis + X-axis - props.setState({ ...props.state, showXAxisTitle: target.checked }) + setState({ ...state, showXAxisTitle: target.checked }) } - checked={props.state?.showXAxisTitle ?? true} + checked={state?.showXAxisTitle ?? true} /> @@ -295,10 +331,8 @@ export function XyToolbar(props: VisualizationToolbarProps) { placeholder={i18n.translate('xpack.lens.xyChart.overwriteXaxis', { defaultMessage: 'Overwrite X-axis title', })} - value={xtitle} - disabled={ - props.state && 'showXAxisTitle' in props.state ? !props.state.showXAxisTitle : false - } + value={xaxistitle} + disabled={state && 'showXAxisTitle' in state ? !state.showXAxisTitle : false} onChange={({ target }) => onXTitleChange(target.value)} aria-label={i18n.translate('xpack.lens.xyChart.overwriteXaxis', { defaultMessage: 'Overwrite X-axis title', @@ -309,18 +343,19 @@ export function XyToolbar(props: VisualizationToolbarProps) { display="columnCompressed" label={ - Y-axis + Y-axis - props.setState({ ...props.state, showYAxisTitle: target.checked }) + setState({ ...state, showYAxisTitle: target.checked }) } - checked={props.state?.showYAxisTitle ?? true} + checked={state?.showYAxisTitle ?? true} /> @@ -332,10 +367,8 @@ export function XyToolbar(props: VisualizationToolbarProps) { placeholder={i18n.translate('xpack.lens.xyChart.overwriteYaxis', { defaultMessage: 'Overwrite Y-axis title', })} - value={ytitle} - disabled={ - props.state && 'showYAxisTitle' in props.state ? !props.state.showYAxisTitle : false - } + value={yaxistitle} + disabled={state && 'showYAxisTitle' in state ? !state.showYAxisTitle : false} onChange={({ target }) => onYTitleChange(target.value)} aria-label={i18n.translate('xpack.lens.xyChart.overwriteYaxis', { defaultMessage: 'Overwrite Y-axis title', 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 40c25ad16717c..548f710e58cd1 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts +++ b/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts @@ -420,8 +420,8 @@ function buildSuggestion({ y: true, }, gridlinesVisibilitySettings: currentState?.gridlinesVisibilitySettings || { - x: false, - y: false, + x: true, + y: true, }, preferredSeriesType: seriesType, layers: Object.keys(existingLayer).length ? keptLayers : [...keptLayers, newLayer], From 593f226790b07e4fbbe9ac3d47389672e5f08c64 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Mon, 10 Aug 2020 11:25:04 +0300 Subject: [PATCH 12/14] disable linter warning --- x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx | 1 + 1 file changed, 1 insertion(+) 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 23224e6f00e84..9ef7384fbdeab 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 @@ -162,6 +162,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { xTitle: defaults.xTitle || x?.label, yTitle: defaults.yTitle || y?.label, }; + // eslint-disable-next-line react-hooks/exhaustive-deps }, [open]); useEffect(() => { From 33de3d165ad5b6dec9f16ac7c1f10f8994fee780 Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Mon, 10 Aug 2020 18:58:00 +0300 Subject: [PATCH 13/14] PR Comments --- .../xy_visualization/xy_config_panel.tsx | 41 +++++++++---------- .../public/xy_visualization/xy_expression.tsx | 6 +-- 2 files changed, 22 insertions(+), 25 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 9ef7384fbdeab..75b02608c4c93 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 @@ -139,13 +139,13 @@ export function XyToolbar(props: VisualizationToolbarProps) { (layer) => layer.seriesType === 'line' || layer.seriesType === 'area' ); - const [xaxistitle, setXaxistitle] = useState(state?.xTitle); - const [yaxistitle, setYaxistitle] = useState(state?.yTitle); + const [xAxisTitle, setXAxisTitle] = useState(state?.xTitle); + const [yAxisTitle, setYAxisTitle] = useState(state?.yTitle); const xyTitles = useCallback(() => { const defaults = { - xTitle: state?.xTitle, - yTitle: state?.yTitle, + xTitle: xAxisTitle, + yTitle: yAxisTitle, }; const layer = state?.layers[0]; if (!layer || !layer.accessors.length) { @@ -170,31 +170,27 @@ export function XyToolbar(props: VisualizationToolbarProps) { xTitle, yTitle, }: { xTitle: string | undefined; yTitle: string | undefined } = xyTitles(); - setXaxistitle(xTitle); - setYaxistitle(yTitle); + setXAxisTitle(xTitle); + setYAxisTitle(yTitle); }, [xyTitles]); - const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState({ - x: state?.tickLabelsVisibilitySettings?.x ?? true, - y: state?.tickLabelsVisibilitySettings?.y ?? true, - }); - const [gridlinesVisibilitySettings, setGridlinesVisibilitySettings] = useState({ - x: state?.gridlinesVisibilitySettings?.x ?? true, - y: state?.gridlinesVisibilitySettings?.y ?? true, - }); - const onXTitleChange = (value: string): void => { - setXaxistitle(value); + setXAxisTitle(value); setState({ ...state, xTitle: value }); }; const onYTitleChange = (value: string): void => { - setYaxistitle(value); + setYAxisTitle(value); setState({ ...state, yTitle: value }); }; type AxesSettingsConfigKeys = keyof AxesSettingsConfig; + const tickLabelsVisibilitySettings = { + x: state?.tickLabelsVisibilitySettings?.x ?? true, + y: state?.tickLabelsVisibilitySettings?.y ?? true, + }; + const onTickLabelsVisibilitySettingsChange = (optionId: string): void => { const id = optionId as AxesSettingsConfigKeys; const newTickLabelsVisibilitySettings = { @@ -203,13 +199,17 @@ export function XyToolbar(props: VisualizationToolbarProps) { [id]: !tickLabelsVisibilitySettings[id], }, }; - setTickLabelsVisibilitySettings(newTickLabelsVisibilitySettings); setState({ ...state, tickLabelsVisibilitySettings: newTickLabelsVisibilitySettings, }); }; + const gridlinesVisibilitySettings = { + x: state?.gridlinesVisibilitySettings?.x ?? true, + y: state?.gridlinesVisibilitySettings?.y ?? true, + }; + const onGridlinesVisibilitySettingsChange = (optionId: string): void => { const id = optionId as AxesSettingsConfigKeys; const newGridlinesVisibilitySettings = { @@ -218,7 +218,6 @@ export function XyToolbar(props: VisualizationToolbarProps) { [id]: !gridlinesVisibilitySettings[id], }, }; - setGridlinesVisibilitySettings(newGridlinesVisibilitySettings); setState({ ...state, gridlinesVisibilitySettings: newGridlinesVisibilitySettings, @@ -425,7 +424,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { placeholder={i18n.translate('xpack.lens.xyChart.overwriteXaxis', { defaultMessage: 'Overwrite X-axis title', })} - value={xaxistitle || ''} + value={xAxisTitle || ''} disabled={state && 'showXAxisTitle' in state ? !state.showXAxisTitle : false} onChange={({ target }) => onXTitleChange(target.value)} aria-label={i18n.translate('xpack.lens.xyChart.overwriteXaxis', { @@ -461,7 +460,7 @@ export function XyToolbar(props: VisualizationToolbarProps) { placeholder={i18n.translate('xpack.lens.xyChart.overwriteYaxis', { defaultMessage: 'Overwrite Y-axis title', })} - value={yaxistitle || ''} + value={yAxisTitle || ''} disabled={state && 'showYAxisTitle' in state ? !state.showYAxisTitle : false} onChange={({ target }) => onYTitleChange(target.value)} aria-label={i18n.translate('xpack.lens.xyChart.overwriteYaxis', { diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx index 2ffd24da33f44..2037a3dbe6623 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx @@ -424,8 +424,7 @@ export function XYChart({ showGridLines={gridlinesVisibilitySettings?.x} gridLineStyle={{ strokeWidth: 2 }} hide={filteredLayers[0].hide} - // @ts-ignore, temporary solution for not displaying the ticks - tickFormat={tickLabelsVisibilitySettings?.x ? (d) => xAxisFormatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings?.x ? (d) => xAxisFormatter.convert(d) : () => ''} /> {yAxesConfiguration.map((axis, index) => ( @@ -437,8 +436,7 @@ export function XYChart({ title={showYAxisTitle ? getYAxesTitles(axis.series, index) : undefined} showGridLines={gridlinesVisibilitySettings?.y} hide={filteredLayers[0].hide} - // @ts-ignore, temporary solution for not displaying the ticks - tickFormat={tickLabelsVisibilitySettings?.y ? (d) => axis.formatter.convert(d) : () => {}} + tickFormat={tickLabelsVisibilitySettings?.y ? (d) => axis.formatter.convert(d) : () => ''} /> ))} From 7b3bc4a18c807c6d438cacad5b56db1f7b89fe4c Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Mon, 10 Aug 2020 19:10:42 +0300 Subject: [PATCH 14/14] Add a comment to callback to explain the decision to listen only to open changes --- .../plugins/lens/public/xy_visualization/xy_config_panel.tsx | 4 ++++ 1 file changed, 4 insertions(+) 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 75b02608c4c93..d64eb9451a50e 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 @@ -162,6 +162,10 @@ export function XyToolbar(props: VisualizationToolbarProps) { xTitle: defaults.xTitle || x?.label, yTitle: defaults.yTitle || y?.label, }; + /* We want this callback to run only if open changes its state. What we want to accomplish here is to give the user a better UX. + By default these input fields have the axis legends. If the user changes the input text, the axis legends should also change. + BUT if the user cleans up the input text, it should remain empty until the user closes and reopens the panel. + In that case, the default axes legend should appear. */ // eslint-disable-next-line react-hooks/exhaustive-deps }, [open]);