[Lens] Use datagrid with resizable columns for datatable #88069
Conversation
dej611
added
release_note:enhancement
Team:Visualizations
|
@elasticmachine merge upstream |
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
While I appreciate the initiative (and love both Lens and Datagrid), I don't feel that the current UX set forth by this PR is in a releasable condition due to the resizing issue. You quickly end up in a situation where the table/grid overflows the Dashboard panel and you essentially can't fix the situation (at least, I couldn't). Presuming that is truly the case, it would be a considerable downgrade from the current basic table approach.
Perhaps there is a way to overcome this such that the datagrid fits the Dashboard panel container, but short of that I would not recommend shipping this change until the issue is resolved.
This was my expectation as well based upon my conversation with Joe - that it would be Lens only, first. |
| @@ -1,3 +1,8 @@ | |||
| .lnsDataTableContainer { | |||
| height: 100%; | |||
| overflow: initial; | |||
There was a problem hiding this comment.
Is this overflow style necessary? It appears to be getting overridden by styles from .lnsVisualizationContainer. I assume it can be removed.
| .lnsDataTableCellContent { | ||
| @include euiTextTruncate; | ||
| } |
There was a problem hiding this comment.
This seems unnecessary, as EuiDataGrid already truncates cell contents. Am I correct in assuming this style was added because the EuiDataGrid truncation stopped working when wrapping the cell contents with div.lnsDataTableCellContent? If that assumption is correct, we can fix this in one of two ways:
- If this
divisn't really necessary, simply removing it will allow theEuiDataGridtruncation to work properly again. - Alternatively, if wrapping element is necessary, changing it to an inline element (such as a
span) should also allow theEuiDataGridtruncation to work properly.
| .lnsDataTable { | ||
| align-self: flex-start; | ||
| } |
There was a problem hiding this comment.
Is this selector being used still? I didn't see it in my testing. If not, should probably be removed.
| .lnsDataTable__filter:focus-within { | ||
| opacity: 1; | ||
| } |
There was a problem hiding this comment.
Is this selector being used still? I didn't see it in my testing. If not, should probably be removed.
|
@elasticmachine merge upstream |
|
@MichaelMarcialis integrated your feedback. I had to use an |
|
@ryankeairns would this PR be ready from your side? |
|
@dej611 I tested a production build with the current version of EUI and for large tables it's definitely much slower than the basic table (for ~130 rows it renders for 2.4 seconds with data grid vs 0.9 seconds with basic table, with the performance optimizations data grid renders for 90 milliseconds). I wouldn't feel comfortable merging it like this as long as we aren't sure we will land the optimizations in 7.12 FYI @chandlerprall |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
dej611
changed the title
Co-authored-by: Joe Reuter <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
… (#89743) Co-authored-by: Joe Reuter <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Fixes #72504, built on top of #87152
This PR migrates the Lens data table visualization to data grid.
New features from previous implementation:
To do:
Considerations
On dashboards we can't persist state at the moment. However it's possible to resize columns - on navigating away from the dashboard, this state is lost.