opensearch-project / OpenSearch-Dashboards

đź“Š Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.67k stars 871 forks source link

[BUG] [TableVis] Individual data grid components don't grow to fit all their rows when filtering increases the number of rows per table #5615

Open joshuarrrr opened 9 months ago

joshuarrrr commented 9 months ago

Describe the bug

When adding a filter, it's common for the number of rows per table to decrease, and the table group layout correctly adjusts to the new table heights. But when that label is removed or updated, the tables continue to be height constrained to a single row height, instead of growing back to their original size.

To Reproduce

See video for reproduction steps.

Expected behavior Individual data grids shouldn't be constrained by previous height settings

OpenSearch Version Please list the version of OpenSearch being used.

Dashboards Version Please list the version of OpenSearch Dashboards being used.

Plugins

Please list all plugins currently enabled.

Screenshots

https://github.com/opensearch-project/OpenSearch-Dashboards/assets/1679762/16437f16-c768-403c-a4ad-7bc294770808

Host/Environment (please complete the following information):

Additional context

Seems possibly related to:

BSFishy commented 9 months ago

The issue here is with the virtualized data grid. From the OUI docs:

Creating a lot of DOM nodes is computationally expensive, and OuiDataGrid uses a couple wrapping divs to build each cell. To help offset the cost of larger tables, cell virtualization can be opted into by constraining the grid's height and/or width. There are two ways to enable this functionality. First, height and/or width can be passed as props, which are applied to the grid's container style. Alternatively, if OuiDataGrid is unable to render at the full dimensions it needs due to screen real estate or other DOM constraints, it will overflow within a scrollable container and only render the visible cells.

I can tell the table is virtualized because it has the euiDataGrid__virtualized class on it. In the doc example, the code says this

          // completely reset the grid when switching between controlled & uncontrolled heights
          // otherwise, going from constrained->unconstrained is ignored.
          // this is for example only, don't switch between controlled & uncontrolled heights
          key={
            height === 'height-unconstrained' ? 'unconstrained' : 'constrained'
          }

When I added key={rows.length} onto this component, the issue went away:

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/a5c45a32b4ac8b3346fb9584e3fe8d30942ba829/src/plugins/vis_type_table/public/components/table_vis_component.tsx#L125-L156

However, this degraded the experience of using the table vis. I think it rebuilds the whole whenever any information changes, which means the screen flashes a few times while the rendering is taking place. So, while this is a solution, I don't feel like it's the correct solution. Potentially properly setting a height or doing some sort of minimum height might work just as well?

joshuarrrr commented 9 months ago

Thanks - yeah, passing the row.length in the key was one of the ideas I came up with but hadn't tested yet. The interesting thing is that internally the data-grid also has some logic specifically tied to changes in the number of rows. But I think it's also an issue of how we're nesting the flex components, because on the initial render, it doesn't seem constrained, but is on subsequent renders.

BSFishy commented 9 months ago

The reason I feel like it's an issue with the virtualization of the data grid, is because the height constraint is on the element that has that class. When I disabled the property that sets the height on that element, the data grid automatically recalculated everything and was laid out correctly, which is why I was on that line of thinking. I feel like the flexbox wouldn't be constraining the height in any way - it's purpose is to be flexible in all dimensions. I could be wrong, but I feel like the issue is in the way the data grid is listening to and handling changes in dimensions and props.

abbyhu2000 commented 7 months ago

Just tested against the 2.12 branch, and this issue with table vis still exists.