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

[7.x] [Lens] Use suggestion system in chart switcher for subtypes (#64613) #64851

Merged
merged 1 commit into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -41,6 +41,10 @@ export const datatableVisualization: Visualization<
},
],

getVisualizationTypeId() {
return 'lnsDatatable';
},

getLayerIds(state) {
return state.layers.map(l => l.layerId);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,25 @@ describe('chart_switch', () => {
id: 'subvisC2',
label: 'C2',
},
{
icon: 'empty',
id: 'subvisC3',
label: 'C3',
},
],
getSuggestions: jest.fn(options => {
if (options.subVisualizationId === 'subvisC2') {
return [];
}
return [
{
score: 1,
title: '',
state: `suggestion`,
previewIcon: 'empty',
},
];
}),
},
};
}
Expand Down Expand Up @@ -313,10 +331,11 @@ describe('chart_switch', () => {
expect(getMenuItem('subvisB', component).prop('betaBadgeIconType')).toBeUndefined();
});

it('should not indicate data loss if visualization is not changed', () => {
it('should not show a warning when the subvisualization is the same', () => {
const dispatch = jest.fn();
const frame = mockFrame(['a', 'b', 'c']);
const visualizations = mockVisualizations();
visualizations.visC.getVisualizationTypeId.mockReturnValue('subvisC2');
const switchVisualizationType = jest.fn(() => 'therebedragons');

visualizations.visC.switchVisualizationType = switchVisualizationType;
Expand All @@ -333,10 +352,10 @@ describe('chart_switch', () => {
/>
);

expect(getMenuItem('subvisC2', component).prop('betaBadgeIconType')).toBeUndefined();
expect(getMenuItem('subvisC2', component).prop('betaBadgeIconType')).not.toBeDefined();
});

it('should remove all layers if there is no suggestion', () => {
it('should get suggestions when switching subvisualization', () => {
const dispatch = jest.fn();
const visualizations = mockVisualizations();
visualizations.visB.getSuggestions.mockReturnValueOnce([]);
Expand Down Expand Up @@ -377,7 +396,7 @@ describe('chart_switch', () => {
const dispatch = jest.fn();
const frame = mockFrame(['a', 'b', 'c']);
const visualizations = mockVisualizations();
const switchVisualizationType = jest.fn(() => 'therebedragons');
const switchVisualizationType = jest.fn(() => 'switched');

visualizations.visC.switchVisualizationType = switchVisualizationType;

Expand All @@ -393,12 +412,12 @@ describe('chart_switch', () => {
/>
);

switchTo('subvisC2', component);
expect(switchVisualizationType).toHaveBeenCalledWith('subvisC2', 'therebegriffins');
switchTo('subvisC3', component);
expect(switchVisualizationType).toHaveBeenCalledWith('subvisC3', 'suggestion');
expect(dispatch).toHaveBeenCalledWith(
expect.objectContaining({
type: 'SWITCH_VISUALIZATION',
initialState: 'therebedragons',
initialState: 'switched',
})
);
expect(frame.removeLayers).not.toHaveBeenCalled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,16 @@ export function ChartSwitch(props: Props) {
const switchVisType =
props.visualizationMap[visualizationId].switchVisualizationType ||
((_type: string, initialState: unknown) => initialState);
if (props.visualizationId === visualizationId) {
const layers = Object.entries(props.framePublicAPI.datasourceLayers);
const containsData = layers.some(
([_layerId, datasource]) => datasource.getTableSpec().length > 0
);
// Always show the active visualization as a valid selection
if (
props.visualizationId === visualizationId &&
props.visualizationState &&
newVisualization.getVisualizationTypeId(props.visualizationState) === subVisualizationId
) {
return {
visualizationId,
subVisualizationId,
Expand All @@ -116,13 +125,13 @@ export function ChartSwitch(props: Props) {
};
}

const layers = Object.entries(props.framePublicAPI.datasourceLayers);
const containsData = layers.some(
([_layerId, datasource]) => datasource.getTableSpec().length > 0
const topSuggestion = getTopSuggestion(
props,
visualizationId,
newVisualization,
subVisualizationId
);

const topSuggestion = getTopSuggestion(props, visualizationId, newVisualization);

let dataLoss: VisualizationSelection['dataLoss'];

if (!containsData) {
Expand Down Expand Up @@ -250,14 +259,16 @@ export function ChartSwitch(props: Props) {
function getTopSuggestion(
props: Props,
visualizationId: string,
newVisualization: Visualization<unknown, unknown>
newVisualization: Visualization<unknown, unknown>,
subVisualizationId?: string
): Suggestion | undefined {
const suggestions = getSuggestions({
datasourceMap: props.datasourceMap,
datasourceStates: props.datasourceStates,
visualizationMap: { [visualizationId]: newVisualization },
activeVisualizationId: props.visualizationId,
visualizationState: props.visualizationState,
subVisualizationId,
}).filter(suggestion => {
// don't use extended versions of current data table on switching between visualizations
// to avoid confusing the user.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export function getSuggestions({
datasourceStates,
visualizationMap,
activeVisualizationId,
subVisualizationId,
visualizationState,
field,
}: {
Expand All @@ -57,6 +58,7 @@ export function getSuggestions({
>;
visualizationMap: Record<string, Visualization>;
activeVisualizationId: string | null;
subVisualizationId?: string;
visualizationState: unknown;
field?: unknown;
}): Suggestion[] {
Expand Down Expand Up @@ -89,7 +91,8 @@ export function getSuggestions({
table,
visualizationId,
datasourceSuggestion,
currentVisualizationState
currentVisualizationState,
subVisualizationId
);
})
)
Expand All @@ -108,13 +111,15 @@ function getVisualizationSuggestions(
table: TableSuggestion,
visualizationId: string,
datasourceSuggestion: DatasourceSuggestion & { datasourceId: string },
currentVisualizationState: unknown
currentVisualizationState: unknown,
subVisualizationId?: string
) {
return visualization
.getSuggestions({
table,
state: currentVisualizationState,
keptLayerIds: datasourceSuggestion.keptLayerIds,
subVisualizationId,
})
.map(({ state, ...visualizationSuggestion }) => ({
...visualizationSuggestion,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/lens/public/editor_frame_service/mocks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export function createMockVisualization(): jest.Mocked<Visualization> {
label: 'TEST',
},
],
getVisualizationTypeId: jest.fn(_state => 'empty'),
getDescription: jest.fn(_state => ({ label: '' })),
switchVisualizationType: jest.fn((_, x) => x),
getPersistableState: jest.fn(_state => _state),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ export const metricVisualization: Visualization<State, PersistableState> = {
},
],

getVisualizationTypeId() {
return 'lnsMetric';
},

clearLayer(state) {
return {
...state,
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugins/lens/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,10 @@ export interface SuggestionRequest<T = unknown> {
* The visualization needs to know which table is being suggested
*/
keptLayerIds: string[];
/**
* Different suggestions can be generated for each subtype of the visualization
*/
subVisualizationId?: string;
}

/**
Expand Down Expand Up @@ -388,6 +392,11 @@ export interface Visualization<T = unknown, P = unknown> {
* but can register multiple subtypes
*/
visualizationTypes: VisualizationType[];
/**
* Return the ID of the current visualization. Used to highlight
* the active subtype of the visualization.
*/
getVisualizationTypeId: (state: T) => string;
/**
* If the visualization has subtypes, update the subtype in state.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function exampleState(): State {
}

describe('xy_visualization', () => {
describe('getDescription', () => {
describe('#getDescription', () => {
function mixedState(...types: SeriesType[]) {
const state = exampleState();
return {
Expand Down Expand Up @@ -81,6 +81,45 @@ describe('xy_visualization', () => {
});
});

describe('#getVisualizationTypeId', () => {
function mixedState(...types: SeriesType[]) {
const state = exampleState();
return {
...state,
layers: types.map((t, i) => ({
...state.layers[0],
layerId: `layer_${i}`,
seriesType: t,
})),
};
}

it('should show mixed when each layer is different', () => {
expect(xyVisualization.getVisualizationTypeId(mixedState('bar', 'line'))).toEqual('mixed');
});

it('should show the preferredSeriesType if there are no layers', () => {
expect(xyVisualization.getVisualizationTypeId(mixedState())).toEqual('bar');
});

it('should combine multiple layers into one type', () => {
expect(
xyVisualization.getVisualizationTypeId(mixedState('bar_horizontal', 'bar_horizontal'))
).toEqual('bar_horizontal');
});

it('should return the subtype for single layers', () => {
expect(xyVisualization.getVisualizationTypeId(mixedState('area'))).toEqual('area');
expect(xyVisualization.getVisualizationTypeId(mixedState('line'))).toEqual('line');
expect(xyVisualization.getVisualizationTypeId(mixedState('area_stacked'))).toEqual(
'area_stacked'
);
expect(xyVisualization.getVisualizationTypeId(mixedState('bar_horizontal_stacked'))).toEqual(
'bar_horizontal_stacked'
);
});
});

describe('#initialize', () => {
it('loads default state', () => {
const mockFrame = createMockFramePublicAPI();
Expand Down
53 changes: 34 additions & 19 deletions x-pack/plugins/lens/public/xy_visualization/xy_visualization.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { I18nProvider } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import { getSuggestions } from './xy_suggestions';
import { LayerContextMenu } from './xy_config_panel';
import { Visualization, OperationMetadata } from '../types';
import { Visualization, OperationMetadata, VisualizationType } from '../types';
import { State, PersistableState, SeriesType, visualizationTypes, LayerConfig } from './types';
import { toExpression, toPreviewExpression } from './to_expression';
import chartBarStackedSVG from '../assets/chart_bar_stacked.svg';
Expand All @@ -24,6 +24,18 @@ const defaultSeriesType = 'bar_stacked';
const isNumericMetric = (op: OperationMetadata) => !op.isBucketed && op.dataType === 'number';
const isBucketed = (op: OperationMetadata) => op.isBucketed;

function getVisualizationType(state: State): VisualizationType | 'mixed' {
if (!state.layers.length) {
return (
visualizationTypes.find(t => t.id === state.preferredSeriesType) ?? visualizationTypes[0]
);
}
const visualizationType = visualizationTypes.find(t => t.id === state.layers[0].seriesType);
const seriesTypes = _.unique(state.layers.map(l => l.seriesType));

return visualizationType && seriesTypes.length === 1 ? visualizationType : 'mixed';
}

function getDescription(state?: State) {
if (!state) {
return {
Expand All @@ -34,39 +46,42 @@ function getDescription(state?: State) {
};
}

const visualizationType = getVisualizationType(state);

if (!state.layers.length) {
const visualizationType = visualizationTypes.find(v => v.id === state.preferredSeriesType)!;
const preferredType = visualizationType as VisualizationType;
return {
icon: visualizationType.largeIcon || visualizationType.icon,
label: visualizationType.label,
icon: preferredType.largeIcon || preferredType.icon,
label: preferredType.label,
};
}

const visualizationType = visualizationTypes.find(t => t.id === state.layers[0].seriesType)!;
const seriesTypes = _.unique(state.layers.map(l => l.seriesType));

return {
icon:
seriesTypes.length === 1
? visualizationType.largeIcon || visualizationType.icon
: chartMixedSVG,
visualizationType === 'mixed'
? chartMixedSVG
: visualizationType.largeIcon || visualizationType.icon,
label:
seriesTypes.length === 1
? visualizationType.label
: isHorizontalChart(state.layers)
? i18n.translate('xpack.lens.xyVisualization.mixedBarHorizontalLabel', {
defaultMessage: 'Mixed horizontal bar',
})
: i18n.translate('xpack.lens.xyVisualization.mixedLabel', {
defaultMessage: 'Mixed XY',
}),
visualizationType === 'mixed'
? isHorizontalChart(state.layers)
? i18n.translate('xpack.lens.xyVisualization.mixedBarHorizontalLabel', {
defaultMessage: 'Mixed horizontal bar',
})
: i18n.translate('xpack.lens.xyVisualization.mixedLabel', {
defaultMessage: 'Mixed XY',
})
: visualizationType.label,
};
}

export const xyVisualization: Visualization<State, PersistableState> = {
id: 'lnsXY',

visualizationTypes,
getVisualizationTypeId(state) {
const type = getVisualizationType(state);
return type === 'mixed' ? type : type.id;
},

getLayerIds(state) {
return state.layers.map(l => l.layerId);
Expand Down