Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Fitting functions #69820

Merged
merged 44 commits into from
Jul 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
bebac29
add toolbar api
flash1293 Jun 16, 2020
e983014
fix teests
flash1293 Jun 16, 2020
3fbff62
fix types
flash1293 Jun 16, 2020
426f2ce
fix functional test
flash1293 Jun 16, 2020
44a17c3
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 18, 2020
8c7381e
add back title
flash1293 Jun 18, 2020
64ee8ef
remove separator line
flash1293 Jun 18, 2020
9bb7843
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 18, 2020
c1ddaaa
fix i18n
flash1293 Jun 18, 2020
e51ff5e
Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/_…
flash1293 Jun 18, 2020
e3c6f94
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 18, 2020
a278772
Merge branch 'lens/toolbar' of github.com:flash1293/kibana into lens/…
flash1293 Jun 18, 2020
bfb6898
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 18, 2020
b3fb6e8
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 18, 2020
2434486
do not show unsaved when there is no cahrt
flash1293 Jun 18, 2020
85c128e
revert unnecessary changes
flash1293 Jun 18, 2020
1ece895
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 23, 2020
34cff58
always show title if available
flash1293 Jun 23, 2020
7a20163
Merge remote-tracking branch 'upstream/master' into lens/toolbar
flash1293 Jun 24, 2020
c6cdca0
fitting functions
flash1293 Jun 24, 2020
c0adafc
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 25, 2020
fc6bd1d
clean up implementation
flash1293 Jun 25, 2020
edf1818
Merge branch 'master' into lens/fitting-functions
elasticmachine Jun 25, 2020
7e744a1
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 26, 2020
5d0b6f4
use compressed form
flash1293 Jun 26, 2020
bde6bec
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 26, 2020
4facefc
Merge branch 'lens/fitting-functions' of github.com:flash1293/kibana …
flash1293 Jun 26, 2020
3c3a4d6
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 26, 2020
6d6f8d0
fix styling
flash1293 Jun 26, 2020
188f3a7
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 29, 2020
4fd45fa
review comments
flash1293 Jun 29, 2020
af9b28e
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jun 30, 2020
cb4c969
exclude stacked area charts
flash1293 Jun 30, 2020
e0d313f
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 1, 2020
62d6945
adjust test and rename none to hidden
flash1293 Jul 1, 2020
b48b8d6
change descriptions
flash1293 Jul 1, 2020
8e9aa69
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 2, 2020
328184f
apply feedback
flash1293 Jul 2, 2020
ba332b3
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 2, 2020
f016ece
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 2, 2020
90fbbb3
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 3, 2020
efd751e
change setting button to text
flash1293 Jul 3, 2020
653ae04
remove unused import
flash1293 Jul 3, 2020
68f961d
Merge remote-tracking branch 'upstream/master' into lens/fitting-func…
flash1293 Jul 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -68,29 +68,33 @@ export function WorkspacePanelWrapper({
return (
<EuiFlexGroup gutterSize="s" direction="column" alignItems="stretch" responsive={false}>
<EuiFlexItem grow={false}>
<ChartSwitch
data-test-subj="lnsChartSwitcher"
visualizationMap={visualizationMap}
visualizationId={visualizationId}
visualizationState={visualizationState}
datasourceMap={datasourceMap}
datasourceStates={datasourceStates}
dispatch={dispatch}
framePublicAPI={framePublicAPI}
/>
<EuiFlexGroup gutterSize="s" direction="row" responsive={false}>
<EuiFlexItem grow={false}>
<ChartSwitch
data-test-subj="lnsChartSwitcher"
visualizationMap={visualizationMap}
visualizationId={visualizationId}
visualizationState={visualizationState}
datasourceMap={datasourceMap}
datasourceStates={datasourceStates}
dispatch={dispatch}
framePublicAPI={framePublicAPI}
/>
</EuiFlexItem>
{activeVisualization && activeVisualization.renderToolbar && (
<EuiFlexItem grow>
<NativeRenderer
render={activeVisualization.renderToolbar}
nativeProps={{
frame: framePublicAPI,
state: visualizationState,
setState: setVisualizationState,
}}
/>
</EuiFlexItem>
)}
</EuiFlexGroup>
</EuiFlexItem>
{activeVisualization && activeVisualization.renderToolbar && (
<EuiFlexItem grow={false}>
<NativeRenderer
render={activeVisualization.renderToolbar}
nativeProps={{
frame: framePublicAPI,
state: visualizationState,
setState: setVisualizationState,
}}
/>
</EuiFlexItem>
)}
<EuiFlexItem>
<EuiPageContent className="lnsWorkspacePanelWrapper">
{(!emptyExpression || title) && (
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

69 changes: 69 additions & 0 deletions x-pack/plugins/lens/public/xy_visualization/fitting_functions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { Fit } from '@elastic/charts';
import { i18n } from '@kbn/i18n';

export type FittingFunction = typeof fittingFunctionDefinitions[number]['id'];

export const fittingFunctionDefinitions = [
{
id: 'None',
title: i18n.translate('xpack.lens.fittingFunctionsTitle.none', {
defaultMessage: 'Hide',
}),
description: i18n.translate('xpack.lens.fittingFunctionsDescription.none', {
defaultMessage: 'Do not fill gaps',
}),
},
{
id: 'Zero',
title: i18n.translate('xpack.lens.fittingFunctionsTitle.zero', {
defaultMessage: 'Zero',
}),
description: i18n.translate('xpack.lens.fittingFunctionsDescription.zero', {
defaultMessage: 'Fill gaps with zeros',
}),
},
{
id: 'Linear',
title: i18n.translate('xpack.lens.fittingFunctionsTitle.linear', {
defaultMessage: 'Linear',
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you swapped the meaning of my upper/lowercase comment and the result is that these are the opposite of what I was expecting. The I18n name should be capitalized for sure, and I also had a preference to have lowercase id fields. As this is currently set up, it looks wrong:

Screenshot 2020-06-30 18 14 02

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that id's should be lowercase, though I do think the visible values should be lowercase as well if you think about an input and it's label being more like sentence, you wouldn't capitalize "None" in a sentence.

description: i18n.translate('xpack.lens.fittingFunctionsDescription.linear', {
defaultMessage: 'Fill gaps with a line',
}),
},
{
id: 'Carry',
title: i18n.translate('xpack.lens.fittingFunctionsTitle.carry', {
defaultMessage: 'Last',
}),
description: i18n.translate('xpack.lens.fittingFunctionsDescription.carry', {
defaultMessage: 'Fill gaps with the last value',
}),
},
{
id: 'Lookahead',
title: i18n.translate('xpack.lens.fittingFunctionsTitle.lookahead', {
defaultMessage: 'Next',
}),
description: i18n.translate('xpack.lens.fittingFunctionsDescription.lookahead', {
defaultMessage: 'Fill gaps with the next value',
}),
},
] as const;
flash1293 marked this conversation as resolved.
Show resolved Hide resolved

export function getFitEnum(fittingFunction?: FittingFunction) {
if (fittingFunction) {
return Fit[fittingFunction];
}
return Fit.None;
}

export function getFitOptions(fittingFunction?: FittingFunction) {
return { type: getFitEnum(fittingFunction) };
}
22 changes: 22 additions & 0 deletions x-pack/plugins/lens/public/xy_visualization/to_expression.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('#toExpression', () => {
{
legend: { position: Position.Bottom, isVisible: true },
preferredSeriesType: 'bar',
fittingFunction: 'Carry',
layers: [
{
layerId: 'first',
Expand All @@ -55,6 +56,27 @@ describe('#toExpression', () => {
).toMatchSnapshot();
});

it('should default the fitting function to None', () => {
expect(
(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).chain[0].arguments.fittingFunction[0]
).toEqual('None');
});

it('should not generate an expression when missing x', () => {
expect(
xyVisualization.toExpression(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export const buildExpression = (
],
},
],
fittingFunction: [state.fittingFunction || 'None'],
layers: validLayers.map((layer) => {
const columnToLabel: Record<string, string> = {};

Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/lens/public/xy_visualization/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import chartBarHorizontalStackedSVG from '../assets/chart_bar_horizontal_stacked
import chartLineSVG from '../assets/chart_line.svg';

import { VisualizationType } from '../index';
import { FittingFunction } from './fitting_functions';

export interface LegendConfig {
isVisible: boolean;
Expand Down Expand Up @@ -225,12 +226,14 @@ export interface XYArgs {
yTitle: string;
legend: LegendConfig & { type: 'lens_xy_legendConfig' };
layers: LayerArgs[];
fittingFunction?: FittingFunction;
}

// Persisted parts of the state
export interface XYState {
preferredSeriesType: SeriesType;
legend: LegendConfig;
fittingFunction?: FittingFunction;
layers: LayerConfig[];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.lnsXyToolbar__popover {
width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using an EUI variable like $euiSize * 24?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something in the back of my mind about not using variables for "arbitrary" sizes that are not tied to padding/margins. $euiSize is important to have paddings and so on line up, but I'm not sure about the value here. cc @cchaos

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
*/

import React from 'react';
import { mountWithIntl as mount } from 'test_utils/enzyme_helpers';
import { EuiButtonGroupProps } from '@elastic/eui';
import { LayerContextMenu } from './xy_config_panel';
import { mountWithIntl as mount, shallowWithIntl as shallow } from 'test_utils/enzyme_helpers';
import { EuiButtonGroupProps, EuiSuperSelect } from '@elastic/eui';
import { LayerContextMenu, XyToolbar } from './xy_config_panel';
import { FramePublicAPI } from '../types';
import { State } from './types';
import { Position } from '@elastic/charts';
import { createMockFramePublicAPI, createMockDatasource } from '../editor_frame_service/mocks';

describe('LayerContextMenu', () => {
describe('XY Config panels', () => {
let frame: FramePublicAPI;

function testState(): State {
Expand All @@ -39,11 +39,6 @@ describe('LayerContextMenu', () => {
};
});

test.skip('allows toggling of legend visibility', () => {});
test.skip('allows changing legend position', () => {});
test.skip('allows toggling the y axis gridlines', () => {});
test.skip('allows toggling the x axis gridlines', () => {});

describe('LayerContextMenu', () => {
test('enables stacked chart types even when there is no split series', () => {
const state = testState();
Expand Down Expand Up @@ -92,4 +87,45 @@ describe('LayerContextMenu', () => {
expect(options!.filter(({ isDisabled }) => isDisabled).map(({ id }) => id)).toEqual([]);
});
});

describe('XyToolbar', () => {
it('should show currently selected fitting function', () => {
const state = testState();

const component = shallow(
<XyToolbar
frame={frame}
setState={jest.fn()}
state={{
...state,
layers: [{ ...state.layers[0], seriesType: 'line' }],
fittingFunction: 'Carry',
}}
/>
);

expect(component.find(EuiSuperSelect).prop('valueOfSelected')).toEqual('Carry');
});

it('should disable the select if there is no unstacked area or line series', () => {
const state = testState();

const component = shallow(
<XyToolbar
frame={frame}
setState={jest.fn()}
state={{
...state,
layers: [
{ ...state.layers[0], seriesType: 'bar' },
{ ...state.layers[0], seriesType: 'area_stacked' },
],
fittingFunction: 'Carry',
}}
/>
);

expect(component.find(EuiSuperSelect).prop('disabled')).toEqual(true);
});
});
});
Loading