-
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
[Data table] Reactify & EUIficate the visualization #70801
Changes from 74 commits
ed29fb2
82712f0
9e17f4d
3a693ef
34a1375
de6dd8f
3a6e113
d2e8436
66654f8
743fded
ca802cd
02013f1
af74780
f6cdb86
c68b60e
dea9e48
d485086
6c489dc
409e34b
14c27a4
5853a10
7a561da
f8accdf
9902674
62ab3c4
0a90deb
a057da9
59987e5
144a7cd
b90d442
d4162fc
04eec45
0bde52a
fe2501f
08944ca
f4790c1
5c5acd7
bc75fef
cd8d67f
9a54372
7816196
73fd7a9
2598656
ea0c16c
395144f
448e23f
89f2e0d
8ae598f
0bc512f
6f0aa33
7836ab1
8d548da
4545764
6372a59
e96b097
445b9ca
1083217
c846aff
61730b3
47b0b0c
1029099
953fdaa
d8a50c9
b51ea9c
df24a7b
27c300f
d21236f
74a71ea
344fdea
03888a0
8c95a55
ae02c6d
7ad54f1
1ace317
7ea584f
8bb70b5
e661af7
a13d027
528865f
ada2cae
3c7950e
ad09412
bb2a885
94706bc
31ee02d
aac4c2d
ef00cc1
17f381d
b4effc4
e502e9d
558d2c7
e993a5e
6ace73a
0320d28
f1cd098
6d620d8
9048a45
5285111
edd4809
6cd3d3d
d9e7c96
d16b81a
26b9cae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,8 @@ | ||
Contains the data table visualization, that allows presenting data in a simple table format. | ||
Contains the data table visualization, that allows presenting data in a simple table format. | ||
|
||
By default a new version of visualization will be used. To use the previous version of visualization the config must have the `vis_type_table.legacyVisEnabled: true` setting | ||
configured in `kibana.dev.yml` or `kibana.yml`, as shown in the example below: | ||
|
||
```yaml | ||
vis_type_table.legacyVisEnabled: true | ||
``` |
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 |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
export { TableOptions } from './table_vis_options_lazy'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import React, { memo, useCallback, useMemo } from 'react'; | ||
import { EuiDataGrid, EuiDataGridSorting, EuiTitle } from '@elastic/eui'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { orderBy } from 'lodash'; | ||
|
||
import { IInterpreterRenderHandlers } from 'src/plugins/expressions'; | ||
import { createTableVisCell } from './table_vis_cell'; | ||
import { Table } from '../table_vis_response_handler'; | ||
import { TableVisConfig, TableVisUiState } from '../types'; | ||
import { useFormattedColumnsAndRows, usePagination } from '../utils'; | ||
import { TableVisControls } from './table_vis_controls'; | ||
import { createGridColumns } from './table_vis_columns'; | ||
|
||
interface TableVisBasicProps { | ||
fireEvent: IInterpreterRenderHandlers['event']; | ||
setSort: (s?: TableVisUiState['sort']) => void; | ||
sort: TableVisUiState['sort']; | ||
table: Table; | ||
visConfig: TableVisConfig; | ||
title?: string; | ||
} | ||
|
||
export const TableVisBasic = memo( | ||
({ fireEvent, setSort, sort, table, visConfig, title }: TableVisBasicProps) => { | ||
const { columns, rows } = useFormattedColumnsAndRows(table, visConfig); | ||
|
||
// custom sorting is in place until the EuiDataGrid sorting gets rid of flaws -> https://github.com/elastic/eui/issues/4108 | ||
const sortedRows = useMemo( | ||
() => | ||
sort.columnIndex !== null && sort.direction | ||
? orderBy(rows, columns[sort.columnIndex]?.id, sort.direction) | ||
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. is this working the same as the old implementation? I checked and it's using 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. Firstly, I don't think there could be boolean values at all, since they come from es response already as numbers (0 and 1). |
||
: rows, | ||
[columns, rows, sort] | ||
); | ||
|
||
// renderCellValue is a component which renders a cell based on column and row indexes | ||
const renderCellValue = useMemo(() => createTableVisCell(columns, sortedRows), [ | ||
columns, | ||
sortedRows, | ||
]); | ||
|
||
// Columns config | ||
const gridColumns = useMemo(() => createGridColumns(table, columns, sortedRows, fireEvent), [ | ||
table, | ||
columns, | ||
sortedRows, | ||
fireEvent, | ||
]); | ||
|
||
// Pagination config | ||
const pagination = usePagination(visConfig); | ||
// Sorting config | ||
const sortingColumns = useMemo( | ||
() => | ||
sort.columnIndex !== null && sort.direction | ||
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. Both sorting and column width is saved by index - this is confusing if the table is extended after configuring this:
The same happens with sorting. The sorting problem exists as well on the legacy table, but I wonder whether we can do better and use some unique id instead. I wouldn't necessarily consider this a blocker, but it's really noticeable for the resizing. 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. Unfortunately, we can't rely on a column's id, since it is dynamically created outside of visualization (from the expression input). |
||
? [{ id: columns[sort.columnIndex]?.id, direction: sort.direction }] | ||
: [], | ||
[columns, sort] | ||
); | ||
const onSort = useCallback( | ||
(sortingCols: EuiDataGridSorting['columns'] | []) => { | ||
// data table vis sorting now only handles one column sorting | ||
// if data grid provides more columns to sort, pick only the next column to sort | ||
const newSortValue = sortingCols.length <= 1 ? sortingCols[0] : sortingCols[1]; | ||
setSort( | ||
newSortValue && { | ||
columnIndex: columns.findIndex((c) => c.id === newSortValue.id), | ||
direction: newSortValue.direction, | ||
} | ||
); | ||
}, | ||
[columns, setSort] | ||
); | ||
|
||
const dataGridAriaLabel = | ||
title || | ||
visConfig.title || | ||
i18n.translate('visTypeTable.defaultAriaLabel', { | ||
defaultMessage: 'Data table visualization', | ||
}); | ||
|
||
return ( | ||
<> | ||
{title && ( | ||
<EuiTitle size="xs"> | ||
<h3>{title}</h3> | ||
</EuiTitle> | ||
)} | ||
<EuiDataGrid | ||
aria-label={dataGridAriaLabel} | ||
columns={gridColumns} | ||
gridStyle={{ | ||
border: 'horizontal', | ||
header: 'underline', | ||
}} | ||
rowCount={rows.length} | ||
columnVisibility={{ | ||
visibleColumns: columns.map(({ id }) => id), | ||
setVisibleColumns: () => {}, | ||
}} | ||
toolbarVisibility={ | ||
visConfig.showToolbar && { | ||
showColumnSelector: false, | ||
showFullScreenSelector: false, | ||
showSortSelector: false, | ||
additionalControls: ( | ||
<TableVisControls | ||
dataGridAriaLabel={dataGridAriaLabel} | ||
cols={columns} | ||
rows={rows} | ||
table={table} | ||
filename={visConfig.title} | ||
/> | ||
), | ||
} | ||
} | ||
renderCellValue={renderCellValue} | ||
renderFooterCellValue={ | ||
visConfig.showTotal | ||
? // @ts-expect-error | ||
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 is concerning - why is this missing in the types? Is this is an oversight or is it intentional and shouldn't be used? In the first case, please open an issue for EUI, otherwise we should work around this and look up the index by the columnId which seems to get passed in. 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. I'll create a pr with fixing this on the eui side. |
||
({ colIndex }) => columns[colIndex].formattedTotal || null | ||
: undefined | ||
} | ||
pagination={pagination} | ||
sorting={{ columns: sortingColumns, onSort }} | ||
/> | ||
</> | ||
); | ||
} | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import React from 'react'; | ||
import { EuiDataGridCellValueElementProps } from '@elastic/eui'; | ||
|
||
import { Table } from '../table_vis_response_handler'; | ||
import { FormattedColumn } from '../types'; | ||
|
||
export const createTableVisCell = (formattedColumns: FormattedColumn[], rows: Table['rows']) => ({ | ||
// @ts-expect-error | ||
colIndex, | ||
sulemanof marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rowIndex, | ||
columnId, | ||
}: EuiDataGridCellValueElementProps) => { | ||
const rowValue = rows[rowIndex][columnId]; | ||
const column = formattedColumns[colIndex]; | ||
const content = column?.formatter?.convert(rowValue, 'html') || (rowValue as string) || ''; | ||
|
||
const cellContent = ( | ||
<div | ||
/* | ||
* Justification for dangerouslySetInnerHTML: | ||
* The Data table visualization can "enrich" cell contents by applying a field formatter, | ||
* which we want to do if possible. | ||
*/ | ||
dangerouslySetInnerHTML={{ __html: content }} // eslint-disable-line react/no-danger | ||
data-test-subj="tbvChartCellContent" | ||
/> | ||
); | ||
|
||
return cellContent; | ||
}; |
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.
I'm not in favor of keeping the legacy data table available behind a flag. I would expect when we reactify and EUIify the data table it simply replaces the old angular one. If you make the old one available behind a flag, then we also have to have telemetry on it to know if users are still using it. If they're still using it, we'd need to know why.
Flags like this which require restarting Kibana also are painful for CI jobs. If we did keep the legacy table behind this flag, I would suggest we stop testing it.
If we're not confident to replace the old data table completely we should wait to merge it until we are confident.
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.
We should def add telemetry to this flag to make sure we understand if people disable this. The reasoning behind the flag was, that data table is the 2nd most used visualization across Kibana, and we're fundamentally exchanging the implementation here. I would like to merge it only once we're certain it's 100% working, but in the past we've seen with a lot of those fundamental refactorings, things still broke and even if it's just some edge-cases only "a handful" of users are having. By that measurement I am not sure I would ever feel "confident" enough to replace it without any fallback flag, or rather feel like we're again using a minor version and our user-case to "test" an implementation for us in all cases.
We've also seen that significant refactorings ever so often required some users to switch back to the old implementation (most recently the conversion from _msearch -> _search might have been a good example were we saw quit some users requiring this flag to fall back).
Ideally this flag only lives for a really short time - I would "timebox" it to max. 2 minor versions (ideally only 1). If you're fine with us not having functional tests for the old implementation in place in favor of not requiring another job runner, we could do that. It just felt safer from my perspective to have at least some very basic tests running.
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.
Running all listed legacy tests will add extra ~20-25 min, that sounds too much for legacy functionality, we will have to re-group several CI groups to optimize overall execution time.
But I think having a few tests with basic checks (rendering data) is a good compromise (5-7 extra minutes to the same CI group 9), we don't want to worry if backward solution works. @sulemanof How about cut some tests from
legacy/data_table/_data_table.ts
?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.
@dmlemeshko @LeeDr
I left only few legacy tests to check basic functionality (basic table, few aggs, filtering, table splitting)
CI time is increased with around 5 extra mins -> https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/86727/execution/node/531/log/?consoleFull
I believe this is quite enough to check the legacy functionality and is not so painful for CI. What do you think?
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.
Personally, I think it is a good way to go.