Skip to content

Commit

Permalink
[Lens] Fix switching with layers (#71982) (#72326)
Browse files Browse the repository at this point in the history
* [Lens] Fix chart switching with multiple layers

* Unskip Lens smokescreen test

* Fix types

* Revert <p> change
  • Loading branch information
Wylie Conlon authored Jul 17, 2020
1 parent ddf0c98 commit b6235ec
Show file tree
Hide file tree
Showing 9 changed files with 214 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ function LayerPanels(
className="lnsConfigPanel__addLayerBtn"
fullWidth
size="s"
data-test-subj="lnsXY_layer_add"
data-test-subj="lnsLayerAddButton"
aria-label={i18n.translate('xpack.lens.xyChart.addLayerButton', {
defaultMessage: 'Add layer',
})}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,14 @@ describe('LayerPanel', () => {
describe('layer reset and remove', () => {
it('should show the reset button when single layer', () => {
const component = mountWithIntl(<LayerPanel {...getDefaultProps()} />);
expect(component.find('[data-test-subj="lns_layer_remove"]').first().text()).toContain(
expect(component.find('[data-test-subj="lnsLayerRemove"]').first().text()).toContain(
'Reset layer'
);
});

it('should show the delete button when multiple layers', () => {
const component = mountWithIntl(<LayerPanel {...getDefaultProps()} isOnlyLayer={false} />);
expect(component.find('[data-test-subj="lns_layer_remove"]').first().text()).toContain(
expect(component.find('[data-test-subj="lnsLayerRemove"]').first().text()).toContain(
'Delete layer'
);
});
Expand All @@ -109,7 +109,7 @@ describe('LayerPanel', () => {
const cb = jest.fn();
const component = mountWithIntl(<LayerPanel {...getDefaultProps()} onRemoveLayer={cb} />);
act(() => {
component.find('[data-test-subj="lns_layer_remove"]').first().simulate('click');
component.find('[data-test-subj="lnsLayerRemove"]').first().simulate('click');
});
expect(cb).toHaveBeenCalled();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ export function LayerPanel(
size="xs"
iconType="trash"
color="danger"
data-test-subj="lns_layer_remove"
data-test-subj="lnsLayerRemove"
onClick={() => {
// If we don't blur the remove / clear button, it remains focused
// which is a strange UX in this case. e.target.blur doesn't work
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ describe('chart_switch', () => {
};
}

/**
* There are three visualizations. Each one has the same suggestion behavior:
*
* visA: suggests an empty state
* visB: suggests an empty state
* visC:
* - Never switches to subvisC2
* - Allows a switch to subvisC3
* - Allows a switch to subvisC1
*/
function mockVisualizations() {
return {
visA: generateVisualization('visA'),
Expand Down Expand Up @@ -292,6 +302,49 @@ describe('chart_switch', () => {
expect(getMenuItem('visB', component).prop('betaBadgeIconType')).toEqual('alert');
});

it('should support multi-layer suggestions without data loss', () => {
const dispatch = jest.fn();
const visualizations = mockVisualizations();
const frame = mockFrame(['a', 'b']);

const datasourceMap = mockDatasourceMap();
datasourceMap.testDatasource.getDatasourceSuggestionsFromCurrentState.mockReturnValue([
{
state: {},
table: {
columns: [
{
columnId: 'a',
operation: {
label: '',
dataType: 'string',
isBucketed: true,
},
},
],
isMultiRow: true,
layerId: 'a',
changeType: 'unchanged',
},
keptLayerIds: ['a', 'b'],
},
]);

const component = mount(
<ChartSwitch
visualizationId="visA"
visualizationState={{}}
visualizationMap={visualizations}
dispatch={dispatch}
framePublicAPI={frame}
datasourceMap={datasourceMap}
datasourceStates={mockDatasourceStates()}
/>
);

expect(getMenuItem('visB', component).prop('betaBadgeIconType')).toBeUndefined();
});

it('should indicate data loss if no data will be used', () => {
const dispatch = jest.fn();
const visualizations = mockVisualizations();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export function ChartSwitch(props: Props) {
dataLoss = 'nothing';
} else if (!topSuggestion) {
dataLoss = 'everything';
} else if (layers.length > 1) {
} else if (layers.length > 1 && layers.length !== topSuggestion.keptLayerIds.length) {
dataLoss = 'layers';
} else if (topSuggestion.columns !== layers[0][1].getTableSpec().length) {
dataLoss = 'columns';
Expand Down Expand Up @@ -258,14 +258,15 @@ function getTopSuggestion(
newVisualization: Visualization<unknown, unknown>,
subVisualizationId?: string
): Suggestion | undefined {
const suggestions = getSuggestions({
const unfilteredSuggestions = getSuggestions({
datasourceMap: props.datasourceMap,
datasourceStates: props.datasourceStates,
visualizationMap: { [visualizationId]: newVisualization },
activeVisualizationId: props.visualizationId,
visualizationState: props.visualizationState,
subVisualizationId,
}).filter((suggestion) => {
});
const suggestions = unfilteredSuggestions.filter((suggestion) => {
// don't use extended versions of current data table on switching between visualizations
// to avoid confusing the user.
return (
Expand Down
92 changes: 86 additions & 6 deletions x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '../types';
import { State, XYState, visualizationTypes } from './types';
import { generateId } from '../id_generator';
import { xyVisualization } from './xy_visualization';

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

Expand Down Expand Up @@ -119,7 +120,33 @@ describe('xy_suggestions', () => {
});

expect(suggestions).toHaveLength(visualizationTypes.length);
expect(suggestions.map(({ state }) => state.preferredSeriesType)).toEqual([
expect(suggestions.map(({ state }) => xyVisualization.getVisualizationTypeId(state))).toEqual([
'bar_stacked',
'area_stacked',
'area',
'line',
'bar_horizontal_stacked',
'bar_horizontal',
'bar',
]);
});

// This limitation is acceptable for now, but is now tested
test('is unable to generate layers when switching from a non-XY chart with multiple layers', () => {
(generateId as jest.Mock).mockReturnValueOnce('aaa');
const suggestions = getSuggestions({
table: {
isMultiRow: true,
columns: [numCol('bytes'), dateCol('date')],
layerId: 'first',
changeType: 'unchanged',
},
keptLayerIds: ['first', 'second'],
});

expect(suggestions).toHaveLength(visualizationTypes.length);
expect(suggestions.map(({ state }) => state.layers.length)).toEqual([1, 1, 1, 1, 1, 1, 1]);
expect(suggestions.map(({ state }) => xyVisualization.getVisualizationTypeId(state))).toEqual([
'bar_stacked',
'area_stacked',
'area',
Expand Down Expand Up @@ -156,7 +183,51 @@ describe('xy_suggestions', () => {
});

expect(suggestions).toHaveLength(visualizationTypes.length);
expect(suggestions.map(({ state }) => state.preferredSeriesType)).toEqual([
expect(suggestions.map(({ state }) => xyVisualization.getVisualizationTypeId(state))).toEqual([
'line',
'bar',
'bar_horizontal',
'bar_stacked',
'bar_horizontal_stacked',
'area',
'area_stacked',
]);
});

test('suggests all basic x y charts when switching from another x y chart with multiple layers', () => {
(generateId as jest.Mock).mockReturnValueOnce('aaa');
const suggestions = getSuggestions({
table: {
isMultiRow: true,
columns: [numCol('bytes'), dateCol('date')],
layerId: 'first',
changeType: 'unchanged',
},
keptLayerIds: ['first', 'second'],
state: {
legend: { isVisible: true, position: 'bottom' },
preferredSeriesType: 'bar',
layers: [
{
layerId: 'first',
seriesType: 'bar',
xAccessor: 'date',
accessors: ['bytes'],
splitAccessor: undefined,
},
{
layerId: 'second',
seriesType: 'bar',
xAccessor: undefined,
accessors: [],
splitAccessor: undefined,
},
],
},
});

expect(suggestions).toHaveLength(visualizationTypes.length);
expect(suggestions.map(({ state }) => xyVisualization.getVisualizationTypeId(state))).toEqual([
'line',
'bar',
'bar_horizontal',
Expand All @@ -165,6 +236,15 @@ describe('xy_suggestions', () => {
'area',
'area_stacked',
]);
expect(suggestions.map(({ state }) => state.layers.map((l) => l.layerId))).toEqual([
['first', 'second'],
['first', 'second'],
['first', 'second'],
['first', 'second'],
['first', 'second'],
['first', 'second'],
['first', 'second'],
]);
});

test('suggests all basic x y chart with date on x', () => {
Expand Down Expand Up @@ -388,7 +468,7 @@ describe('xy_suggestions', () => {
changeType: 'unchanged',
},
state: currentState,
keptLayerIds: [],
keptLayerIds: ['first'],
});

expect(rest).toHaveLength(visualizationTypes.length - 2);
Expand Down Expand Up @@ -497,7 +577,7 @@ describe('xy_suggestions', () => {
changeType: 'extended',
},
state: currentState,
keptLayerIds: [],
keptLayerIds: ['first'],
});

expect(rest).toHaveLength(0);
Expand Down Expand Up @@ -536,7 +616,7 @@ describe('xy_suggestions', () => {
changeType: 'reorder',
},
state: currentState,
keptLayerIds: [],
keptLayerIds: ['first'],
});

expect(rest).toHaveLength(0);
Expand Down Expand Up @@ -576,7 +656,7 @@ describe('xy_suggestions', () => {
changeType: 'extended',
},
state: currentState,
keptLayerIds: [],
keptLayerIds: ['first'],
});

expect(rest).toHaveLength(0);
Expand Down
16 changes: 12 additions & 4 deletions x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,17 +394,25 @@ function buildSuggestion({
: undefined,
};

// Maintain consistent order for any layers that were saved
const keptLayers = currentState
? currentState.layers.filter(
(layer) => layer.layerId !== layerId && keptLayerIds.includes(layer.layerId)
)
? currentState.layers
// Remove layers that aren't being suggested
.filter((layer) => keptLayerIds.includes(layer.layerId))
// Update in place
.map((layer) => (layer.layerId === layerId ? newLayer : layer))
// Replace the seriesType on all previous layers
.map((layer) => ({
...layer,
seriesType,
}))
: [];

const state: State = {
legend: currentState ? currentState.legend : { isVisible: true, position: Position.Right },
fittingFunction: currentState?.fittingFunction || 'None',
preferredSeriesType: seriesType,
layers: [...keptLayers, newLayer],
layers: Object.keys(existingLayer).length ? keptLayers : [...keptLayers, newLayer],
};

return {
Expand Down
26 changes: 26 additions & 0 deletions x-pack/test/functional/apps/lens/smokescreen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,5 +165,31 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
// legend item(s), so we're using a class selector here.
expect(await find.allByCssSelector('.echLegendItem')).to.have.length(3);
});

it('should switch from a multi-layer stacked bar to a multi-layer line chart', async () => {
await PageObjects.visualize.navigateToNewVisualization();
await PageObjects.visualize.clickVisType('lens');
await PageObjects.lens.goToTimeRange();

await PageObjects.lens.configureDimension({
dimension: 'lnsXY_xDimensionPanel > lns-empty-dimension',
operation: 'date_histogram',
field: '@timestamp',
});

await PageObjects.lens.configureDimension({
dimension: 'lnsXY_yDimensionPanel > lns-empty-dimension',
operation: 'avg',
field: 'bytes',
});

await PageObjects.lens.createLayer();

expect(await PageObjects.lens.hasChartSwitchWarning('line')).to.eql(false);

await PageObjects.lens.switchToVisualization('line');

expect(await PageObjects.lens.getLayerCount()).to.eql(2);
});
});
}
28 changes: 28 additions & 0 deletions x-pack/test/functional/page_objects/lens_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,5 +167,33 @@ export function LensPageProvider({ getService, getPageObjects }: FtrProviderCont
await testSubjects.existOrFail('visTypeTitle');
});
},

/**
* Checks a specific subvisualization in the chart switcher for a "data loss" indicator
*
* @param subVisualizationId - the ID of the sub-visualization to switch to, such as
* lnsDatatable or bar_stacked
*/
async hasChartSwitchWarning(subVisualizationId: string) {
await this.openChartSwitchPopover();

const element = await testSubjects.find(`lnsChartSwitchPopover_${subVisualizationId}`);
return await testSubjects.descendantExists('euiKeyPadMenuItem__betaBadgeWrapper', element);
},

/**
* Returns the number of layers visible in the chart configuration
*/
async getLayerCount() {
const elements = await testSubjects.findAll('lnsLayerRemove');
return elements.length;
},

/**
* Adds a new layer to the chart, fails if the chart does not support new layers
*/
async createLayer() {
await testSubjects.click('lnsLayerAddButton');
},
});
}

0 comments on commit b6235ec

Please sign in to comment.