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

[EuiDataGrid] Add Row Summary #8002

Open
Tracked by #182611
timductive opened this issue Sep 4, 2024 · 11 comments
Open
Tracked by #182611

[EuiDataGrid] Add Row Summary #8002

timductive opened this issue Sep 4, 2024 · 11 comments

Comments

@timductive
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

timductive commented Sep 4, 2024

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

@cee-chen
Copy link
Member

cee-chen commented Sep 4, 2024

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: bvaughn/react-window#425 and bvaughn/react-window#60 (comment).

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
Copy link

dimadavid commented Sep 4, 2024

cc @paulewing @YulNaumenko

@logeekal
Copy link

logeekal commented Sep 5, 2024

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
Copy link
Member

cee-chen commented Sep 6, 2024

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
Copy link

logeekal commented Sep 10, 2024

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
Copy link
Member Author

@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
Copy link

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.
list_virtualization_prototype_sorting.mov

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.

@logeekal
Copy link

logeekal commented Sep 18, 2024

Heyy @timductive ,

Virtualization is now working with decent performance. See below demo in timeline.

event_renderer_100_rows.mov

This technique does not let us enable column virtualization. But it is okay I think unless users add huge number of columns.

Hey @cee-chen ,

I was able to leverage react-window only to add this virtualization since we are already using it. I am still on the fence whether it should be in EUI or in the code that implements EUIDataGrid. But i think we can first test it extensively in security solution and then can think of adding it in EUI.

However, to enable this I have raised this PR ( #8028 ) for few minor changes in EUI. Currently, it is draft as i am still changing docs. But I will try to finalize it this week.

Currently, I do not know whow move a change in EUI API with solutions also adapting that change in single PR. Is there a process for it? Please let me know if this needs more discussion.

@cee-chen
Copy link
Member

This technique does not let us enable column virtualization. But it is okay I think unless users add huge number of columns.

This feels like a solid compromise for sure! Awesome thinking! 👏

I am still on the fence whether it should be in EUI or in the code that implements EUIDataGrid

++ to what you said - if production usage goes well in terms of bugs or performance, we can discuss bringing the functionality back into EuiDataGrid later. But it feels much safer to only roll this out for a specific use case/datagrid rather than every single datagrid across Elastic/Kibana.

However, to enable this I have raised this PR ( #8028 ) for few minor changes in EUI. [...] Currently, I do not know whow move a change in EUI API with solutions also adapting that change in single PR

There's no way to release something in both EUI and Kibana at the same time. There's a week or so lag time between EUI main and Kibana main, where EUI handles the release and any Kibana-impacting changes in our weekly upgrades.

If you want to test Kibana production usage before your code merges into EUI main, we have a wiki doc that should walk you through how to do so here: https://github.com/elastic/eui/blob/main/wiki/contributing-to-eui/testing/testing-in-kibana.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants