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.

Sign up for GitHub

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] Fix switching with layers #71982

Merged
merged 7 commits into from
Jul 17, 2020
Merged

[Lens] Fix switching with layers #71982

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9819acb
[Lens] Fix chart switching with multiple layers
wylieconlon Jul 15, 2020
a32f4c1
Merge remote-tracking branch 'origin/master' into lens/fix-switching-…
wylieconlon Jul 15, 2020
3ed4b61
Unskip Lens smokescreen test
wylieconlon Jul 15, 2020
a678514
Merge remote-tracking branch 'origin/master' into lens/fix-switching-…
wylieconlon Jul 16, 2020
a41ea50
Fix types
wylieconlon Jul 16, 2020
9c55cd3
Merge remote-tracking branch 'origin/master' into lens/fix-switching-…
wylieconlon Jul 17, 2020
bac7af1
Revert <p> change
wylieconlon Jul 17, 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
2 changes: 1 addition & 1 deletion x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions ...k/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx
View file
Open in desktop
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
44 changes: 44 additions & 0 deletions ...ugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.test.tsx
View file
Open in desktop
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,40 @@ 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: ['2'],
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
7 changes: 4 additions & 3 deletions ...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions ...plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,14 +202,14 @@ export function InnerWorkspacePanel({
url={core.http.basePath.prepend(emptyStateGraphicURL)}
alt=""
/>
<p>
<div>
<FormattedMessage
id="xpack.lens.editorFrame.emptyWorkspaceHeading"
defaultMessage="Lens is a new tool for creating visualizations"
/>{' '}
<EuiBetaBadge label="Beta" tooltipContent={tooltipContent} />
</p>
<p>
</div>
<div>
<small>
<EuiLink
href="https://www.elastic.co/products/kibana/feedback"
Expand All @@ -222,7 +222,7 @@ export function InnerWorkspacePanel({
/>
</EuiLink>
</small>
</p>
</div>
Copy link
Contributor Author

@wylieconlon wylieconlon Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This <p> wrapping a <div> was causing errors in tests.

Before:

Screenshot 2020-07-16 13 04 37

After:

Screenshot 2020-07-15 17 09 25

Copy link
Contributor

Choose a reason for hiding this comment

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

😕 ? What does this fix? Using paragraphs is better DOM rendering for screen readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fixes this warning:

console.error
      Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>.
          in div (created by EuiIcon)
          in EuiIcon (created by Context.Consumer)
          in EuiI18n (created by EuiLink)
          in a (created by EuiLink)
          in EuiLink (created by InnerWorkspacePanel)
          in small (created by InnerWorkspacePanel)
          in p (created by InnerWorkspacePanel)

Copy link
Contributor

Choose a reason for hiding this comment

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

Those all stem from EuiIcon and it'll be hard to get rid of them until we change the element that EuiIcon renders. I'd rather keep the appropriate text elements for screen-readers when possible and I'll make an issue for this in EUI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok this is weird, EuiIcon does not in fact ever render a <div> (I think it did way back when, but now it's just a simple <svg>). So this error message is actually completely false. Do we know who setup this DOM validator so I can follow up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so I think I've tracked this down to the fact that in Kibana, because EuiIcon is dynamically imported, jest tests mock the component into rendering a <div>. So the React warning is actually remarking on the snapshots and not the actual dom. I'll see if the mocks are also coming from EUI, and if so, just get them to render a span.

But in the meantime, can you revert this p to div change? If you truly like the minimized space between the two lines, then wrap them all in a single <p> with a <br /> in between.

</EuiText>
</div>
);
Expand Down
18 changes: 9 additions & 9 deletions x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,11 @@ describe('IndexPatternDimensionEditorPanel', () => {
const items: EuiListGroupItemProps[] = wrapper.find(EuiListGroup).prop('listItems') || [];

expect(items.find(({ label }) => label === 'Minimum')!['data-test-subj']).not.toContain(
'Incompatible'
'incompatible'
);

expect(items.find(({ label }) => label === 'Date histogram')!['data-test-subj']).toContain(
'Incompatible'
'incompatible'
);
});

Expand Down Expand Up @@ -601,7 +601,7 @@ describe('IndexPatternDimensionEditorPanel', () => {

act(() => {
wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');
});

Expand All @@ -612,7 +612,7 @@ describe('IndexPatternDimensionEditorPanel', () => {
wrapper = mount(<IndexPatternDimensionEditorComponent {...defaultProps} />);

wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');

expect(wrapper.find('[data-test-subj="indexPattern-invalid-operation"]')).not.toHaveLength(
Expand All @@ -626,7 +626,7 @@ describe('IndexPatternDimensionEditorPanel', () => {
wrapper = mount(<IndexPatternDimensionEditorComponent {...defaultProps} />);

wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');

wrapper
Expand All @@ -640,7 +640,7 @@ describe('IndexPatternDimensionEditorPanel', () => {
wrapper = mount(<IndexPatternDimensionEditorComponent {...defaultProps} />);

wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');

const options = wrapper
Expand Down Expand Up @@ -722,7 +722,7 @@ describe('IndexPatternDimensionEditorPanel', () => {
);

wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-count"]')
.find('button[data-test-subj="lns-indexPatternDimension-count incompatible"]')
.simulate('click');

const newColumnState = setState.mock.calls[0][0].layers.first.columns.col2;
Expand Down Expand Up @@ -758,7 +758,7 @@ describe('IndexPatternDimensionEditorPanel', () => {
);

wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');

const options = wrapper
Expand All @@ -781,7 +781,7 @@ describe('IndexPatternDimensionEditorPanel', () => {

act(() => {
wrapper
.find('button[data-test-subj="lns-indexPatternDimensionIncompatible-terms"]')
.find('button[data-test-subj="lns-indexPatternDimension-terms incompatible"]')
.simulate('click');
});

Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,9 @@ export function PopoverEditor(props: PopoverEditorProps) {
isActive,
size: 's',
className: 'lnsIndexPatternDimensionEditor__operation',
'data-test-subj': `lns-indexPatternDimension${
compatibleWithCurrentField ? '' : 'Incompatible'
}-${operationType}`,
'data-test-subj': `lns-indexPatternDimension-${operationType}${
compatibleWithCurrentField ? '' : ' incompatible'
}`,
onClick() {
if (!selectedColumn || !compatibleWithCurrentField) {
const possibleFields = fieldByOperation[operationType] || [];
Expand Down
92 changes: 86 additions & 6 deletions x-pack/plugins/lens/public/xy_visualization/xy_suggestions.test.ts
View file
Open in desktop
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
Loading