-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Trigger a filter action on click in datatable visualization #63840
Changes from 11 commits
be57365
7475198
a6183a6
abe63e1
093c6c7
af89e27
9647389
d6a65bc
632e096
a57d980
e4c85bc
7d3d93a
badca7d
6a94994
fb8c322
1615f23
594210d
35deebb
3e9b878
dab164a
47d0e1b
1dcdfa2
ea231fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,14 @@ | ||||||||
.lnsDataTable { | ||||||||
align-self: flex-start; | ||||||||
} | ||||||||
|
||||||||
.lnsDataTable__filter { | ||||||||
opacity: 0; | ||||||||
transition: opacity 250ms ease-in-out; | ||||||||
} | ||||||||
|
||||||||
.lnsDataTable__cell:hover { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The icons need to reveal on focus as well.
Suggested change
|
||||||||
.lnsDataTable__filter { | ||||||||
opacity: 1; | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
/* | ||
* 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 React from 'react'; | ||
import { shallow } from 'enzyme'; | ||
import { mountWithIntl } from 'test_utils/enzyme_helpers'; | ||
import { datatable, DatatableComponent } from './expression'; | ||
import { LensMultiTable } from '../types'; | ||
import { DatatableProps } from './expression'; | ||
import { createMockExecutionContext } from '../../../../../src/plugins/expressions/common/mocks'; | ||
import { IFieldFormat } from '../../../../../src/plugins/data/public'; | ||
|
||
const executeTriggerActions = jest.fn(); | ||
|
||
function sampleArgs() { | ||
const data: LensMultiTable = { | ||
type: 'lens_multitable', | ||
tables: { | ||
l1: { | ||
type: 'kibana_datatable', | ||
columns: [ | ||
{ id: 'a', name: 'a', meta: { type: 'count' } }, | ||
{ id: 'b', name: 'b', meta: { type: 'date_histogram' } }, | ||
{ id: 'c', name: 'c', meta: { type: 'cardinality' } }, | ||
], | ||
rows: [{ a: 10110, b: 2, c: 3 }], | ||
}, | ||
}, | ||
}; | ||
|
||
const args: DatatableProps['args'] = { | ||
title: 'My fanci metric chart', | ||
columns: { | ||
columnIds: ['a', 'b', 'c'], | ||
type: 'lens_datatable_columns', | ||
}, | ||
}; | ||
|
||
return { data, args }; | ||
} | ||
|
||
describe('datatable_expression', () => { | ||
describe('datatable renders', () => { | ||
test('it renders with the specified data and args', () => { | ||
const { data, args } = sampleArgs(); | ||
const result = datatable.fn(data, args, createMockExecutionContext()); | ||
|
||
expect(result).toEqual({ | ||
type: 'render', | ||
as: 'lens_datatable_renderer', | ||
value: { data, args }, | ||
}); | ||
}); | ||
}); | ||
|
||
describe('DatatableComponent', () => { | ||
test('it renders the title and value', () => { | ||
const { data, args } = sampleArgs(); | ||
|
||
expect( | ||
shallow( | ||
<DatatableComponent | ||
data={data} | ||
args={args} | ||
formatFactory={x => x as IFieldFormat} | ||
executeTriggerActions={executeTriggerActions} | ||
getType={jest.fn()} | ||
/> | ||
) | ||
).toMatchSnapshot(); | ||
}); | ||
|
||
test('it invokes executeTriggerActions with correct context', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to be missing a test involving date fields |
||
const { args, data } = sampleArgs(); | ||
|
||
const wrapper = mountWithIntl( | ||
<DatatableComponent | ||
data={data} | ||
args={args} | ||
formatFactory={x => x as IFieldFormat} | ||
executeTriggerActions={executeTriggerActions} | ||
getType={jest.fn(() => ({ type: 'buckets' }))} | ||
/> | ||
); | ||
|
||
wrapper | ||
.find('[data-test-subj="lensDatatableFilterOut"]') | ||
.first() | ||
.simulate('click'); | ||
|
||
expect(executeTriggerActions).toHaveBeenCalledWith('VALUE_CLICK_TRIGGER', { | ||
data: { | ||
data: [ | ||
{ | ||
column: 0, | ||
row: 0, | ||
table: data.tables.l1, | ||
value: 10110, | ||
}, | ||
], | ||
negate: true, | ||
}, | ||
timeFieldName: undefined, | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,19 @@ | |
import React from 'react'; | ||
import ReactDOM from 'react-dom'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { EuiBasicTable } from '@elastic/eui'; | ||
import { I18nProvider, FormattedMessage } from '@kbn/i18n/react'; | ||
import { EuiBasicTable, EuiFlexGroup, EuiButtonIcon, EuiFlexItem, EuiToolTip } from '@elastic/eui'; | ||
import { FormatFactory, LensMultiTable } from '../types'; | ||
import { | ||
ExpressionFunctionDefinition, | ||
ExpressionRenderDefinition, | ||
IInterpreterRenderHandlers, | ||
} from '../../../../../src/plugins/expressions/public'; | ||
import { VisualizationContainer } from '../visualization_container'; | ||
import { ValueClickTriggerContext } from '../../../../../src/plugins/embeddable/public'; | ||
import { VIS_EVENT_TO_TRIGGER } from '../../../../../src/plugins/visualizations/public'; | ||
import { UiActionsStart } from '../../../../../src/plugins/ui_actions/public'; | ||
import { getExecuteTriggerActions } from '../services'; | ||
|
||
export interface DatatableColumns { | ||
columnIds: string[]; | ||
|
@@ -30,6 +35,12 @@ export interface DatatableProps { | |
args: Args; | ||
} | ||
|
||
type DatatableRenderProps = DatatableProps & { | ||
formatFactory: FormatFactory; | ||
executeTriggerActions: UiActionsStart['executeTriggerActions']; | ||
getType: Function; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this type stronger? it's not just a function, it's a function that returns an AggType? |
||
}; | ||
|
||
export interface DatatableRender { | ||
type: 'render'; | ||
as: 'lens_datatable_renderer'; | ||
|
@@ -100,9 +111,10 @@ export const datatableColumns: ExpressionFunctionDefinition< | |
}, | ||
}; | ||
|
||
export const getDatatableRenderer = ( | ||
formatFactory: Promise<FormatFactory> | ||
): ExpressionRenderDefinition<DatatableProps> => ({ | ||
export const getDatatableRenderer = (dependencies: { | ||
formatFactory: Promise<FormatFactory>; | ||
getType: Promise<Function>; | ||
}): ExpressionRenderDefinition<DatatableProps> => ({ | ||
name: 'lens_datatable_renderer', | ||
displayName: i18n.translate('xpack.lens.datatable.visualizationName', { | ||
defaultMessage: 'Datatable', | ||
|
@@ -115,9 +127,18 @@ export const getDatatableRenderer = ( | |
config: DatatableProps, | ||
handlers: IInterpreterRenderHandlers | ||
) => { | ||
const resolvedFormatFactory = await formatFactory; | ||
const resolvedFormatFactory = await dependencies.formatFactory; | ||
const executeTriggerActions = getExecuteTriggerActions(); | ||
const resolvedGetType = await dependencies.getType; | ||
ReactDOM.render( | ||
<DatatableComponent {...config} formatFactory={resolvedFormatFactory} />, | ||
<I18nProvider> | ||
<DatatableComponent | ||
{...config} | ||
formatFactory={resolvedFormatFactory} | ||
executeTriggerActions={executeTriggerActions} | ||
getType={resolvedGetType} | ||
/> | ||
</I18nProvider>, | ||
domNode, | ||
() => { | ||
handlers.done(); | ||
|
@@ -127,14 +148,38 @@ export const getDatatableRenderer = ( | |
}, | ||
}); | ||
|
||
function DatatableComponent(props: DatatableProps & { formatFactory: FormatFactory }) { | ||
export function DatatableComponent(props: DatatableRenderProps) { | ||
const [firstTable] = Object.values(props.data.tables); | ||
const formatters: Record<string, ReturnType<FormatFactory>> = {}; | ||
|
||
firstTable.columns.forEach(column => { | ||
formatters[column.id] = props.formatFactory(column.formatHint); | ||
}); | ||
|
||
const handleFilterClick = (field: string, value: unknown, index: number, negate = false) => { | ||
const timeFieldName = negate | ||
? undefined | ||
: firstTable.columns.find(col => col?.meta?.type === 'date_histogram')?.meta?.aggConfigParams | ||
?.field; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This approach assumes that the column being clicked is the first date column. The table can have two or more date columns in any order, so the timeFieldName is not necessarily first. Try the following scenario:
|
||
const rowIndex = firstTable.rows.findIndex(row => row[field] === value); | ||
|
||
const context: ValueClickTriggerContext = { | ||
data: { | ||
negate, | ||
data: [ | ||
{ | ||
row: rowIndex, | ||
column: index, | ||
value, | ||
table: firstTable, | ||
}, | ||
], | ||
}, | ||
timeFieldName, | ||
}; | ||
props.executeTriggerActions(VIS_EVENT_TO_TRIGGER.filter, context); | ||
}; | ||
|
||
return ( | ||
<VisualizationContainer> | ||
<EuiBasicTable | ||
|
@@ -144,23 +189,86 @@ function DatatableComponent(props: DatatableProps & { formatFactory: FormatFacto | |
columns={props.args.columns.columnIds | ||
.map(field => { | ||
const col = firstTable.columns.find(c => c.id === field); | ||
const colIndex = firstTable.columns.findIndex(c => c.id === field); | ||
|
||
const filterable = col?.meta?.type && props.getType(col.meta.type)?.type === 'buckets'; | ||
return { | ||
field, | ||
name: (col && col.name) || '', | ||
render: (value: unknown) => { | ||
const formattedValue = formatters[field]?.convert(value); | ||
if (filterable && value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The condition here is preventing filters from being added when the unformatted value is empty string or 0: both of those are valid categories. |
||
return ( | ||
<EuiFlexGroup | ||
className="lnsDataTable__cell" | ||
data-test-subj="lnsDataTableCellValueFilterable" | ||
gutterSize="xs" | ||
> | ||
<EuiFlexItem grow={false}>{formattedValue}</EuiFlexItem> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've chosen not to put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Along with @cchaos we've decided for aligning with the inspector look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It keeps the filter actions closer to the actual value (the text) which helps in not requiring a long distance to travel between value and actions. |
||
<EuiFlexItem grow={false}> | ||
<EuiFlexGroup | ||
responsive={false} | ||
gutterSize="none" | ||
alignItems="center" | ||
className="lnsDataTable__filter" | ||
> | ||
<EuiToolTip | ||
position="bottom" | ||
content={ | ||
<FormattedMessage | ||
id="xpack.lens.filterForValueButtonTooltip" | ||
defaultMessage="Filter for value" | ||
/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I'm not sure we need to use a |
||
} | ||
> | ||
<EuiButtonIcon | ||
iconType="plusInCircle" | ||
color="text" | ||
aria-label={i18n.translate( | ||
'xpack.lens.filterForValueButtonAriaLabel', | ||
{ | ||
defaultMessage: 'Filter for value', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the aria-labels only, I'm wondering if we should include the value here like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
} | ||
)} | ||
data-test-subj="lensDatatableFilterFor" | ||
onClick={() => handleFilterClick(field, value, colIndex)} | ||
/> | ||
</EuiToolTip> | ||
<EuiFlexItem grow={false}> | ||
<EuiToolTip | ||
position="bottom" | ||
content={ | ||
<FormattedMessage | ||
id="xpack.lens.filterOutValueButtonTooltip" | ||
defaultMessage="Filter out value" | ||
/> | ||
} | ||
> | ||
<EuiButtonIcon | ||
iconType="minusInCircle" | ||
color="text" | ||
aria-label={i18n.translate( | ||
'xpack.lens.filterOutValueButtonAriaLabel', | ||
{ | ||
defaultMessage: 'Filter out value', | ||
} | ||
)} | ||
data-test-subj="lensDatatableFilterOut" | ||
onClick={() => handleFilterClick(field, value, colIndex, true)} | ||
/> | ||
</EuiToolTip> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
); | ||
} | ||
return <span data-test-subj="lnsDataTableCellValue">{formattedValue}</span>; | ||
}, | ||
}; | ||
}) | ||
.filter(({ field }) => !!field)} | ||
items={ | ||
firstTable | ||
? firstTable.rows.map(row => { | ||
const formattedRow: Record<string, unknown> = {}; | ||
Object.entries(formatters).forEach(([columnId, formatter]) => { | ||
formattedRow[columnId] = formatter.convert(row[columnId]); | ||
}); | ||
return formattedRow; | ||
}) | ||
: [] | ||
} | ||
items={firstTable ? firstTable.rows : []} | ||
/> | ||
</VisualizationContainer> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other tables don't seem to have a transition on the tooltip, why add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styles are aligned with inspector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.