elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.08k stars 829 forks source link

[EuiDataGrid] Add Row Summary #8002

Open timductive opened 1 week ago

timductive commented 1 week ago

Is your feature request related to a problem? Please describe. In Discover datagrid, we need to be able to summarize a document by adding a full-width row cell below the regular row.

This is done in Security for summarizing Events:

Image

timductive commented 1 week ago

This looks difficult from a performance concern, it sounds like the underlying datagrid library does not offer the feature to expand a cell across all columns like this. We need a rough estimate on effort and implementation plan here before we can agree if this work is worth doing.

timductive commented 1 week ago

Tagging a few folks for visibility: @ninoslavmiskovic @sophiec20 @dimadavid @logeekal @michaelolo24 @kertal @cee-chen @JasonStoltz @yanwalton

cee-chen commented 1 week ago

it sounds like the underlying datagrid library does not offer the feature to expand a cell across all columns like this.

Correct, it's a limitation of the underlying virtualization library. See: https://github.com/bvaughn/react-window/issues/425 and https://github.com/bvaughn/react-window/issues/60#issuecomment-479953599.

An alternative approach could be looking for another virtualization library (or rolling your own) that supports this, and then using a custom grid body renderer (which puts the onus of a one-off use case on the implementing team, but still gains the team the benefits of EUI's toolbars/headers/footers/sorting/cell actions etc).

To be a little spicy: I think this once again begs the question of whether a data grid component/library should fall under the purview of a design system team or whether it needs to be split off to its own team, as I consider this to be a very large engineering lift.

dimadavid commented 1 week ago

cc @paulewing @YulNaumenko

logeekal commented 1 week ago

An alternative approach could be looking for another virtualization library (or rolling your own) that supports this, and then using a custom grid body renderer (which puts the onus of a one-off use case on the implementing team, but still gains the team the benefits of EUI's toolbars/headers/footers/sorting/cell actions etc).

Hi @cee-chen , Actually I did this experimentation with tanstack/react-virtual and our custom virtualization but the results were disappointing w.r.t to rendering of rows. I will try to implement it again in code-sandbox so that you or someone from your team can may be take a look.

I have been, alternatively, thinking of an alternative approach. Today, Eui gives control of custom body render because it re-uses trailingControlColumns and consumers can render trailingControlColumn in whatever way they want.

What if we can have a prop dedicated for a full-width column and there is only one way that can be rendered. With that, that full-width column actually be the part of row ( from perspective of react-window ) and there is not need for separate implementation of customBodyRender, we can have fixed implementation inside EuiDataGrid table code where One ReactWindow Row === ActualRow + Rowrenderer. I am not sure if this can get around the limitation of react-window.

What do you think? Please let me know if I was not clear.

cee-chen commented 1 week ago

What if we can have a prop dedicated for a full-width column and there is only one way that can be rendered. With that, that full-width column actually be the part of row ( from perspective of react-window ) and there is not need for separate implementation of customBodyRender, we can have fixed implementation inside EuiDataGrid table code where One ReactWindow Row === ActualRow + Rowrenderer. I am not sure if this can get around the limitation of react-window.

Theoretically this is feasible if we know the height of this full width cell/'column' in advance, and if that height is a static number.

Unfortunately if it's a dynamic height per row, it runs into the same performance/implementation issues that we run into with auto height datagrid rows: we needing to measure the height of the row and then setting it manually/rerendering it on every resize. If we do not, the virtualization library will not correctly absolutely position every other adjacent/sibling row/cell.

logeekal commented 5 days ago

Unfortunately if it's a dynamic height per row, it runs into the same performance/implementation issues that we run into with auto height datagrid rows: we needing to measure the height of the row and then setting it manually/rerendering it on every resize. If we do not, the virtualization library will not correctly absolutely position every other adjacent/sibling row/cell.

That is what I have been trying to research. I have just created this below PoC which does List virtualization ( using react-virtualized) of with cells of dynamic height. I took EUI example component for Custom Body Render from docs.

https://codesandbox.io/p/sandbox/custom-body-render-react-virtualized-cell-measurer-3jhqfl?file=%2FmeasuredCell.tsx%3A161%2C49&workspaceId=53bdfd6b-eb5e-4ca0-a3fc-fb193014a4ab

There are 2 questions to be answered before this could be implemented in production

  1. Cell component, that is being rendered on line 187-191 in measuredCell.tsx does not have correct dimension when it is rendered. If I replace the extra row with static paragraph, virtualization works pretty well. I still need to see what is the reason for that and how can we correct it. You can actually replicate it by commenting line 187 and uncommenting line 188-191.

  2. The performance implications. In this simple example performance seems good enough. But i still need to benchmark it for our real world use-case.

Additonally, Currently column virtualization is not implemented but only row virtualization. But i think it can be easily implemented using the same technique of list virtualization.

I am starting to make some changes in EUI to see how it can support our use cases. Please let me know what you think.

timductive commented 4 days ago

@logeekal Some additional questions:

  1. Is this an accessible table structure?
  2. Does sorting/filtering fields work? They don't seem to work in the POC but not sure if that is just a detail of the prototype.
logeekal commented 3 days ago

Hey @timductive ,

  1. Is this an accessible table structure?
  2. Does sorting/filtering fields work? They don't seem to work in the POC but not sure if that is just a detail of the prototype.
  1. I will need to check for accessibility later when i am sure it can be put in production. Currently, if we use table structure like this, EUI anyway delegates the responsibility of accesibility to solution that use EUIDataGrid.
  2. Sorting is working for me in the prototype ( see below video ). Were you trying to sort on 2 columns because then it might look like it is not working because all values are distinct. Also, All functionalities of EUIDataGrid should work as I am not changing any particular logic or structure of EUIDataGrid. This is only how things are rendered so functionalities should not be affected.

https://github.com/user-attachments/assets/29e3c68f-8b1b-499e-9934-111ba7f8a3f0

I will be working on below issues in order :

  1. [ In progress ]There is some issue with height calculation issue with dynamic rows.
  2. Once that is done, then I will benchmark the performance to make the decision whether we can use it or not. So far it looks good.
  3. Accessibility.

will keep this thread updated.