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] Make operation order more clear to users #48305

Merged
merged 6 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -176,5 +176,39 @@ describe('Datatable Visualization', () => {
],
});
});

it('reorders the rendered colums based on the order from the datasource', () => {
const datasource = createMockDatasource();
const layer = { layerId: 'a', columns: ['b', 'c'] };
const frame = mockFrame();
frame.datasourceLayers = { a: datasource.publicAPIMock };
const component = mount(
<DataTableLayer
dragDropContext={{ dragging: undefined, setDragging: () => {} }}
frame={frame}
layer={layer}
setState={jest.fn()}
state={{ layers: [layer] }}
/>
);

const accessors = component
.find('[data-test-subj="datatable_multicolumnEditor"]')
.first()
.prop('accessors') as string[];

expect(accessors).toEqual(['b', 'c']);

component.setProps({
layer: { layerId: 'a', columns: ['c', 'b'] },
});

const newAccessors = component
.find('[data-test-subj="datatable_multicolumnEditor"]')
.first()
.prop('accessors') as string[];

expect(newAccessors).toEqual(['c', 'b']);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ export function DataTableLayer({
dragDropContext,
}: { layer: LayerState } & VisualizationProps<DatatableVisualizationState>) {
const datasource = frame.datasourceLayers[layer.layerId];

const originalOrder = datasource.getTableSpec().map(({ columnId }) => columnId);
// When we add a column it could be empty, and therefore have no order
const sortedColumns = originalOrder.concat(
layer.columns.filter(id => {
return !originalOrder.includes(id);
})
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified, I think:

const sortedColumns = Array.from(new Set(originalOrder.concat(layer.columns)));

return (
<EuiPanel className="lnsConfigPanel__panel" paddingSize="s">
<NativeRenderer
Expand All @@ -71,7 +80,7 @@ export function DataTableLayer({
label={i18n.translate('xpack.lens.datatable.columns', { defaultMessage: 'Columns' })}
>
<MultiColumnEditor
accessors={layer.columns}
accessors={sortedColumns}
datasource={datasource}
dragDropContext={dragDropContext}
filterOperations={allOperations}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('BucketNestingEditor', () => {
return result as IndexPatternColumn;
}

it('should display an unchecked switch if there are two buckets and it is the root', () => {
it('should display the top level grouping when at the root', () => {
const component = mount(
<BucketNestingEditor
columnId="a"
Expand All @@ -45,12 +45,14 @@ describe('BucketNestingEditor', () => {
setColumns={jest.fn()}
/>
);
const control = component.find('[data-test-subj="indexPattern-nesting-switch"]').first();
const control1 = component.find('[data-test-subj="indexPattern-nesting-topLevel"]').first();
const control2 = component.find('[data-test-subj="indexPattern-nesting-bottomLevel"]').first();

expect(control.prop('checked')).toBeFalsy();
expect(control1.prop('checked')).toBeTruthy();
expect(control2.prop('checked')).toBeFalsy();
});

it('should display a checked switch if there are two buckets and it is not the root', () => {
it('should display the bottom level grouping when appropriate', () => {
const component = mount(
<BucketNestingEditor
columnId="a"
Expand All @@ -66,9 +68,12 @@ describe('BucketNestingEditor', () => {
setColumns={jest.fn()}
/>
);
const control = component.find('[data-test-subj="indexPattern-nesting-switch"]').first();

expect(control.prop('checked')).toBeTruthy();
const control1 = component.find('[data-test-subj="indexPattern-nesting-topLevel"]').first();
const control2 = component.find('[data-test-subj="indexPattern-nesting-bottomLevel"]').first();

expect(control1.prop('checked')).toBeFalsy();
expect(control2.prop('checked')).toBeTruthy();
});

it('should reorder the columns when toggled', () => {
Expand All @@ -88,11 +93,31 @@ describe('BucketNestingEditor', () => {
setColumns={setColumns}
/>
);
const control = component.find('[data-test-subj="indexPattern-nesting-switch"]').first();
const control1 = component.find('[data-test-subj="indexPattern-nesting-topLevel"]').first();

(control.prop('onChange') as () => {})();
(control1.prop('onChange') as () => {})();

expect(setColumns).toHaveBeenCalledTimes(1);
expect(setColumns).toHaveBeenCalledWith(['a', 'b', 'c']);

component.setProps({
layer: {
columnOrder: ['a', 'b', 'c'],
columns: {
a: mockCol({ suggestedPriority: 0 }),
b: mockCol({ suggestedPriority: 1 }),
c: mockCol({ suggestedPriority: 2, operationType: 'min', isBucketed: false }),
},
indexPatternId: 'foo',
},
});

const control2 = component.find('[data-test-subj="indexPattern-nesting-bottomLevel"]').first();

(control2.prop('onChange') as () => {})();

expect(setColumns).toHaveBeenCalledTimes(2);
expect(setColumns).toHaveBeenLastCalledWith(['b', 'a', 'c']);
});

it('should display nothing if there are no buckets', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@
import _ from 'lodash';
import React from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFormRow, EuiHorizontalRule, EuiSwitch, EuiSelect } from '@elastic/eui';
import { EuiFormRow, EuiHorizontalRule, EuiRadio, EuiSelect, htmlIdGenerator } from '@elastic/eui';
import { IndexPatternLayer } from '../types';
import { hasField } from '../utils';

const generator = htmlIdGenerator('lens-nesting');

function nestColumn(columnOrder: string[], outer: string, inner: string) {
const result = columnOrder.filter(c => c !== inner);
Expand All @@ -32,35 +35,75 @@ export function BucketNestingEditor({
const columns = Object.entries(layer.columns);
const aggColumns = columns
.filter(([id, c]) => id !== columnId && c.isBucketed)
.map(([value, c]) => ({ value, text: c.label }));
.map(([value, c]) => ({
value,
text: c.label,
fieldName: hasField(c) ? c.sourceField : '',
}));

if (!column || !column.isBucketed || !aggColumns.length) {
return null;
}

const fieldName = hasField(column) ? column.sourceField : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same logic as above. Not a big deal. But might be something we can tidy up later.


const prevColumn = layer.columnOrder[layer.columnOrder.indexOf(columnId) - 1];

if (aggColumns.length === 1) {
const [target] = aggColumns;

function toggleNesting() {
if (prevColumn) {
setColumns(nestColumn(layer.columnOrder, columnId, target.value));
} else {
setColumns(nestColumn(layer.columnOrder, target.value, columnId));
}
}

return (
<>
<EuiHorizontalRule margin="m" />
<EuiSwitch
data-test-subj="indexPattern-nesting-switch"
label={i18n.translate('xpack.lens.xyChart.nestUnderTarget', {
defaultMessage: 'Nest under {target}',
values: { target: target.text },
<EuiFormRow
label={i18n.translate('xpack.lens.indexPattern.groupingControlLabel', {
defaultMessage: 'Grouping',
})}
checked={!!prevColumn}
onChange={() => {
if (prevColumn) {
setColumns(nestColumn(layer.columnOrder, columnId, target.value));
} else {
setColumns(nestColumn(layer.columnOrder, target.value, columnId));
}
}}
/>
>
<>
<EuiRadio
id={generator()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this, but is this predictive, or is it going to cause render churn? It seems totally fine to me to just specify an ID schematically here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I'm supposed to pass a string, and it'll generate consistent ids if given a string.

data-test-subj="indexPattern-nesting-topLevel"
label={
column.operationType === 'terms'
? i18n.translate('xpack.lens.indexPattern.groupingOverallTerms', {
defaultMessage: 'Overall top {field}',
values: { field: fieldName },
})
: i18n.translate('xpack.lens.indexPattern.groupingOverallDateHistogram', {
defaultMessage: 'Dates overall',
})
}
checked={!prevColumn}
onChange={toggleNesting}
/>
<EuiRadio
id={generator()}
data-test-subj="indexPattern-nesting-bottomLevel"
label={
column.operationType === 'terms'
? i18n.translate('xpack.lens.indexPattern.groupingSecondTerms', {
defaultMessage: 'Top values for each {target}',
values: { target: target.fieldName },
})
: i18n.translate('xpack.lens.indexPattern.groupingSecondDateHistogram', {
defaultMessage: 'Dates for each {target}',
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this verbiage pretty confusing. I think {field} for each {target} or something would be clearer.

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 don't think it improves the readability to use field names like that- especially when the editor is showing the current field in multiple places.

values: { target: target.fieldName },
})
}
checked={!!prevColumn}
onChange={toggleNesting}
/>
</>
</EuiFormRow>
</>
);
}
Expand All @@ -69,8 +112,8 @@ export function BucketNestingEditor({
<>
<EuiHorizontalRule margin="m" />
<EuiFormRow
label={i18n.translate('xpack.lens.xyChart.nestUnder', {
defaultMessage: 'Nest under',
label={i18n.translate('xpack.lens.indexPattern.groupByDropdown', {
defaultMessage: 'Group by',
})}
display="rowCompressed"
>
Expand All @@ -81,7 +124,7 @@ export function BucketNestingEditor({
{
value: '',
text: i18n.translate('xpack.lens.xyChart.nestUnderRoot', {
defaultMessage: 'Top level',
defaultMessage: 'Entire data set',
}),
},
...aggColumns,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export function PopoverEditor(props: PopoverEditorProps) {
layerId,
currentIndexPattern,
uniqueLabel,
hideGrouping,
} = props;
const { operationByDocument, operationByField, fieldByOperation } = operationFieldSupportMatrix;
const [isPopoverOpen, setPopoverOpen] = useState(false);
Expand Down Expand Up @@ -393,22 +394,25 @@ export function PopoverEditor(props: PopoverEditorProps) {
/>
</EuiFormRow>
)}
<BucketNestingEditor
layer={state.layers[props.layerId]}
columnId={props.columnId}
setColumns={columnOrder => {
setState({
...state,
layers: {
...state.layers,
[props.layerId]: {
...state.layers[props.layerId],
columnOrder,

{!hideGrouping && (
<BucketNestingEditor
layer={state.layers[props.layerId]}
columnId={props.columnId}
setColumns={columnOrder => {
setState({
...state,
layers: {
...state.layers,
[props.layerId]: {
...state.layers[props.layerId],
columnOrder,
},
},
},
});
}}
/>
});
}}
/>
)}
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,9 +442,9 @@ function createMetricSuggestion(

function getNestedTitle([outerBucket, innerBucket]: IndexPatternColumn[]) {
return i18n.translate('xpack.lens.indexpattern.suggestions.nestingChangeLabel', {
defaultMessage: '{innerOperation} per each {outerOperation}',
defaultMessage: '{innerOperation} for each {outerOperation}',
values: {
innerOperation: innerBucket.label,
innerOperation: hasField(innerBucket) ? innerBucket.sourceField : innerBucket.label,
outerOperation: hasField(outerBucket) ? outerBucket.sourceField : outerBucket.label,
},
});
Expand Down
5 changes: 5 additions & 0 deletions x-pack/legacy/plugins/lens/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ export interface DatasourceDimensionPanelProps {
// affects the default ordering of the query
suggestedPriority?: DimensionPriority;
onRemove?: (accessor: string) => void;

// Some dimension editors will allow users to change the operation grouping
// from the panel, and this lets the visualization hint that it doesn't want
// users to have that level of control
hideGrouping?: boolean;
}

export interface DatasourceLayerPanelProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ export function XYConfigPanel(props: VisualizationProps<State>) {
filterOperations: isBucketed,
suggestedPriority: 1,
layerId: layer.layerId,
hideGrouping: true,
}}
/>
</EuiFormRow>
Expand Down