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] Add xy split series support #39726

Merged
merged 13 commits into from
Jul 10, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@ import {
import { mount } from 'enzyme';
import { act } from 'react-dom/test-utils';
import { Operation, DataType } from '../types';
import * as generator from '../id_generator';
import { mockGeneratedIds } from '../id_generator/mock';

jest.mock('../id_generator');

describe('Datatable Visualization', () => {
describe('#initialize', () => {
it('should initialize from the empty state', () => {
const datasource = createMockDatasource();
datasource.publicAPIMock.generateColumnId.mockReturnValueOnce('id');
mockGeneratedIds(generator, 'id');
expect(datatableVisualization.initialize(datasource.publicAPIMock)).toEqual({
columns: [{ id: 'id', label: '' }],
});
Expand All @@ -30,7 +34,6 @@ describe('Datatable Visualization', () => {
const expectedState: DatatableVisualizationState = {
columns: [{ id: 'saved', label: 'label' }],
};
expect(datasource.publicAPIMock.generateColumnId).not.toHaveBeenCalled();
expect(datatableVisualization.initialize(datasource.publicAPIMock, expectedState)).toEqual(
expectedState
);
Expand Down Expand Up @@ -138,7 +141,7 @@ describe('Datatable Visualization', () => {
/>
);

datasource.publicAPIMock.generateColumnId.mockReturnValueOnce('newId');
mockGeneratedIds(generator, 'newId');

act(() => {
wrapper
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
VisualizationSuggestion,
} from '../types';
import { NativeRenderer } from '../native_renderer';
import { generateId } from '../id_generator';

export interface DatatableVisualizationState {
columns: Array<{
Expand Down Expand Up @@ -117,7 +118,7 @@ export function DatatableConfigPanel(props: VisualizationProps<DatatableVisualiz
onClick={() => {
const newColumns = [...state.columns];
newColumns.push({
id: datasource.generateColumnId(),
id: generateId(),
label: '',
});
setState({
Expand All @@ -141,7 +142,7 @@ export const datatableVisualization: Visualization<
state || {
columns: [
{
id: datasource.generateColumnId(),
id: generateId(),
label: '',
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,6 @@ Object {
setDatasourceState(updatedState);
});

expect(mockDatasource.getPublicAPI).toHaveBeenCalledTimes(2);
expect(mockDatasource.getPublicAPI).toHaveBeenLastCalledWith(
updatedState,
expect.any(Function)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import React, { useState, useEffect } from 'react';
import { FormattedMessage } from '@kbn/i18n/react';

import { EuiIcon, EuiTitle, EuiPanel, EuiIconTip } from '@elastic/eui';
import { toExpression } from '@kbn/interpreter/common';
import { i18n } from '@kbn/i18n';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export function createMockDatasource(): DatasourceMock {
const publicAPIMock: jest.Mocked<DatasourcePublicAPI> = {
getTableSpec: jest.fn(() => []),
getOperationForColumnId: jest.fn(),
generateColumnId: jest.fn(),
renderDimensionPanel: jest.fn(),
removeColumnInTableSpec: jest.fn(),
moveColumnTo: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* 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 { generateId } from './id_generator';

describe('XYConfigPanel', () => {
it('generates different ids', () => {
expect(generateId()).not.toEqual(generateId());
});
});
11 changes: 11 additions & 0 deletions x-pack/legacy/plugins/lens/public/id_generator/id_generator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* 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 uuid from 'uuid/v4';

export function generateId() {
return uuid();
}
7 changes: 7 additions & 0 deletions x-pack/legacy/plugins/lens/public/id_generator/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* 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.
*/

export * from './id_generator';
10 changes: 10 additions & 0 deletions x-pack/legacy/plugins/lens/public/id_generator/mock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* 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.
*/

export function mockGeneratedIds(generator: { generateId: () => string }, ...ids: string[]) {
const mock = jest.spyOn(generator, 'generateId');
ids.forEach(id => mock.mockReturnValueOnce(id));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you take advantage of jest's ability to replace modules with manual mocks? https://jestjs.io/docs/en/manual-mocks

Here's what I was thinking:

In id_generator/__mocks__/index.ts:

export function generateId() {
  return jest.fn();
}

In your test code:

import { generateId } from '../id_generator';

jest.mock('../id_generator');

  test('allows adding y dimensions', () => {
    (generateId as jest.Mock).mockReturnValueOnce('zed');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks much nicer. I'll give it a shot.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { render } from 'react-dom';
import { Chrome } from 'ui/chrome';
import { ToastNotifications } from 'ui/notify/toasts/toast_notifications';
import { EuiComboBox } from '@elastic/eui';
import uuid from 'uuid';
import { Datasource, DataType } from '..';
import {
DatasourceDimensionPanelProps,
Expand Down Expand Up @@ -262,11 +261,6 @@ export function getIndexPatternDatasource(chrome: Chrome, toastNotifications: To
}
return columnToOperation(state.columns[columnId]);
},
generateColumnId: () => {
// TODO: Come up with a more compact form of generating unique column ids
return uuid.v4();
},

renderDimensionPanel: (domElement: Element, props: DatasourceDimensionPanelProps) => {
render(
<IndexPatternDimensionPanel
Expand Down
1 change: 0 additions & 1 deletion x-pack/legacy/plugins/lens/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ export interface Datasource<T = unknown, P = unknown> {
export interface DatasourcePublicAPI {
getTableSpec: () => TableSpec;
getOperationForColumnId: (columnId: string) => Operation | null;
generateColumnId: () => string;

// Render can be called many times
renderDimensionPanel: (domElement: Element, props: DatasourceDimensionPanelProps) => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import { DatasourcePublicAPI, DatasourceDimensionPanelProps, Operation } from '.
import { State, SeriesType } from './types';
import { Position } from '@elastic/charts';
import { NativeRendererProps } from '../native_renderer';
import * as generator from '../id_generator';
import { mockGeneratedIds } from '../id_generator/mock';

jest.mock('../id_generator');

describe('XYConfigPanel', () => {
const dragDropContext = { dragging: undefined, setDragging: jest.fn() };
Expand All @@ -20,7 +24,6 @@ describe('XYConfigPanel', () => {
return {
duplicateColumn: () => [],
getOperationForColumnId: () => null,
generateColumnId: () => 'TESTID',
getTableSpec: () => [],
moveColumnTo: () => {},
removeColumnInTableSpec: () => [],
Expand Down Expand Up @@ -357,12 +360,13 @@ describe('XYConfigPanel', () => {
});

test('allows adding y dimensions', () => {
mockGeneratedIds(generator, 'zed');
const setState = jest.fn();
const state = testState();
const component = mount(
<XYConfigPanel
dragDropContext={dragDropContext}
datasource={{ ...mockDatasource(), generateColumnId: () => 'zed' }}
datasource={mockDatasource()}
setState={setState}
state={{ ...state, y: { ...state.y, accessors: ['a', 'b', 'c'] } }}
/>
Expand All @@ -376,6 +380,27 @@ describe('XYConfigPanel', () => {
});
});

test('allows adding split dimensions', () => {
mockGeneratedIds(generator, 'foo');
const setState = jest.fn();
const state = testState();
const component = mount(
<XYConfigPanel
dragDropContext={dragDropContext}
datasource={mockDatasource()}
setState={setState}
state={{ ...state, splitSeriesAccessors: ['a', 'b', 'c'] }}
/>
);

(testSubj(component, 'lnsXY_splitSeriesDimensionPanel_add').onClick as Function)();

expect(setState).toHaveBeenCalledTimes(1);
expect(setState.mock.calls[0][0]).toMatchObject({
splitSeriesAccessors: ['a', 'b', 'c', 'foo'],
});
});

test('allows toggling the y axis gridlines', () => {
const toggleYGridlines = (showGridlines: boolean) => {
const setState = jest.fn();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
import { State, SeriesType } from './types';
import { VisualizationProps, Operation } from '../types';
import { NativeRenderer } from '../native_renderer';
import { generateId } from '../id_generator';

const chartTypeIcons: Array<{ id: SeriesType; label: string; iconType: IconType }> = [
{
Expand Down Expand Up @@ -161,6 +162,37 @@ export function XYConfigPanel(props: VisualizationProps<State>) {
</EuiFormRow>
)}

<EuiFormRow
label={i18n.translate('xpack.lens.xyChart.splitSeries', {
defaultMessage: 'Split series',
})}
>
<>
{state.splitSeriesAccessors.map(columnId => (
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a way to remove existing split column like for y columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can:

image

That's the split series list.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisdavies That just deletes the column in the data source, not the accessor in the visualization. If you press the plus button ten times, you have ten columns "to configure" and now way to remove those placeholders.
Screenshot 2019-06-28 at 19 27 44

y axis has separate remove buttons but they are not styled nicely
Screenshot 2019-06-28 at 19 28 06

<NativeRenderer
data-test-subj="lnsXY_splitDimensionPanel"
render={datasource.renderDimensionPanel}
nativeProps={{
columnId,
dragDropContext: props.dragDropContext,
filterOperations: (op: Operation) => op.isBucketed === true,
suggestedPriority: 0,
}}
/>
))}
<EuiButton
data-test-subj="lnsXY_splitSeriesDimensionPanel_add"
onClick={() =>
setState({
...state,
splitSeriesAccessors: [...state.splitSeriesAccessors, generateId()],
})
}
iconType="plusInCircle"
/>
</>
</EuiFormRow>

<EuiFormRow
label={i18n.translate('xpack.lens.xyChart.xAxisLabel', {
defaultMessage: 'X Axis',
Expand Down Expand Up @@ -287,7 +319,7 @@ export function XYConfigPanel(props: VisualizationProps<State>) {
...state,
y: {
...state.y,
accessors: [...state.y.accessors, datasource.generateColumnId()],
accessors: [...state.y.accessors, generateId()],
},
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import { getSuggestions } from './xy_suggestions';
import { TableColumn, VisualizationSuggestion } from '../types';
import { State } from './types';
import { Ast } from '@kbn/interpreter/target/common';
import * as generator from '../id_generator';
import { mockGeneratedIds } from '../id_generator/mock';

jest.mock('../id_generator');

describe('xy_suggestions', () => {
function numCol(columnId: string): TableColumn {
Expand Down Expand Up @@ -88,6 +92,7 @@ describe('xy_suggestions', () => {
});

test('suggests a basic x y chart with date on x', () => {
mockGeneratedIds(generator, 'aaa');
const [suggestion, ...rest] = getSuggestions({
tables: [
{
Expand All @@ -102,7 +107,9 @@ describe('xy_suggestions', () => {
expect(suggestionSubset(suggestion)).toMatchInlineSnapshot(`
Object {
"seriesType": "line",
"splitSeriesAccessors": Array [],
"splitSeriesAccessors": Array [
"aaa",
],
"stackAccessors": Array [],
"x": "date",
"y": Array [
Expand Down Expand Up @@ -141,6 +148,7 @@ Object {
});

test('supports multiple suggestions', () => {
mockGeneratedIds(generator, 'bbb', 'ccc');
const [s1, s2, ...rest] = getSuggestions({
tables: [
{
Expand All @@ -161,7 +169,9 @@ Object {
Array [
Object {
"seriesType": "line",
"splitSeriesAccessors": Array [],
"splitSeriesAccessors": Array [
"bbb",
],
"stackAccessors": Array [],
"x": "date",
"y": Array [
Expand All @@ -170,7 +180,9 @@ Array [
},
Object {
"seriesType": "bar",
"splitSeriesAccessors": Array [],
"splitSeriesAccessors": Array [
"ccc",
],
"stackAccessors": Array [],
"x": "country",
"y": Array [
Expand All @@ -182,6 +194,7 @@ Array [
});

test('handles two numeric values', () => {
mockGeneratedIds(generator, 'ddd');
const [suggestion] = getSuggestions({
tables: [
{
Expand All @@ -195,7 +208,9 @@ Array [
expect(suggestionSubset(suggestion)).toMatchInlineSnapshot(`
Object {
"seriesType": "bar",
"splitSeriesAccessors": Array [],
"splitSeriesAccessors": Array [
"ddd",
],
"stackAccessors": Array [],
"x": "quantity",
"y": Array [
Expand All @@ -206,6 +221,7 @@ Object {
});

test('handles unbucketed suggestions', () => {
mockGeneratedIds(generator, 'eee');
const [suggestion] = getSuggestions({
tables: [
{
Expand All @@ -230,7 +246,9 @@ Object {
expect(suggestionSubset(suggestion)).toMatchInlineSnapshot(`
Object {
"seriesType": "bar",
"splitSeriesAccessors": Array [],
"splitSeriesAccessors": Array [
"eee",
],
"stackAccessors": Array [],
"x": "mybool",
"y": Array [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { partition } from 'lodash';
import { Position } from '@elastic/charts';
import { SuggestionRequest, VisualizationSuggestion, TableColumn, TableSuggestion } from '../types';
import { State } from './types';
import { generateId } from '../id_generator';
import { buildExpression } from './to_expression';

const columnSortOrder = {
Expand Down Expand Up @@ -80,7 +81,7 @@ function getSuggestion(
title,
legend: { isVisible: true, position: Position.Right },
seriesType: isDate ? 'line' : 'bar',
splitSeriesAccessors: splitBy && isDate ? [splitBy.columnId] : [],
splitSeriesAccessors: splitBy && isDate ? [splitBy.columnId] : [generateId()],
stackAccessors: splitBy && !isDate ? [splitBy.columnId] : [],
x: {
accessor: xValue.columnId,
Expand Down
Loading