opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
35 stars 69 forks source link

[Proposal][Draft] Auto fit row heights based on cell context in OuiDataGrid #1218

Open ananzh opened 8 months ago

ananzh commented 8 months ago

Objective

To enhance the OuiDataGrid component from the OUI framework by implementing an "auto-fit" feature for row heights. This feature will automatically adjust the height of each row based on its content, improving the display of multiline or variable-sized content.

Current Implementation

Currently, row heights in OuiDataGrid are set using the rowHeightsOptions object, which allows for specifying a fixed number of lines (lineCount) or a fixed height for rows. This implementation provides basic control but lacks the flexibility needed for content that varies significantly in size.

Proposed Change

Add auto option in defaultHeight in rowHeightsOptions

This change will enable the grid to dynamically calculate and adjust row heights based on the content of each cell. Customers can update rowHeightsOptions:

 const rowHeightsOptions = useMemo(
    () => ({
      defaultHeight: adjustedColumns.includes('_source')? 'auto' : {
        lineCount: 1,
      },
    }),
    [adjustedColumns]
  );

Enhance RowHeightUtils

Update the RowHeightUtils class to better support dynamic height calculations. Add methods for measuring content height in each cell and caching these measurements for performance optimization.

Modify OuiDataGridBody

Adjust OuiDataGridCell and OuiDataGridCellContent

Ensure that cells can dynamically adjust their size based on content. Implement a size auto observer within cells to detect content size changes and trigger row height adjustments.

BSFishy commented 8 months ago

I would love to have this feature in. It would certainly bring datagrid a lot closer to a table-like component. My only concern would be with performance. Datagrid is already a heavy component, then you're adding a height calculation on every single cell on top of that.

I'm not super familiar with how datagrid works under the hood, but even with memoization, you're needing to render content -> wait until the browser lays it out -> get the height of each cell -> get the max cell height of the row -> update the position of each cell in the current row and every row after the current row. That, by itself, is 1 or 2 additional re-renders, assuming none of this laying out causes side effects that will trigger more renders.

Correct me if I'm wrong! Again, I'm not super familiar with the internals of datagrid, so I could very well be mistaken!

To be quite honest, I don't know why any of the layout logic needs to be happening in JS. It seems to me like the layout is a pretty rigidly defined, solved problem. You have column widths that are defined by the header, and potentially variable height rows that are consistent across the rows. There is no reason, in my mind that this couldn't be taken care of by the browser.

I feel like a situation like this could work very well:

Screenshot (198)

Where we essentially have a bunch of rows stacked on top of each other. These rows span the entire width of the datagrid. We don't even need to worry about their height (unless the caller explicitly configures a row height), as the browser can figure that stuff out for us. Within each row, we have a set of cells. Each cell fills the vertical space, and has an explicit width parameter that matches the column.

I think this would be a significant departure from how datagrid is currently implemented, but I feel like it could performance and variable height rows.

Thoughts?

ananzh commented 8 months ago

@BSFishy Thanks for the feedbacks and I really like your idea: Currently for rowHeightUtils, instead of calculating and setting heights for each cell, the grid could allow the browser's CSS layout engine to determine the necessary height for each row based on its content. I do think about how we could implemented.

However, the decision to implement auto-fit row heights in OuiDataGrid as an intermediate step before allowing users to manually adjust row heights via the toolbar involves weighing the pros and cons of each approach. This is like part of the customization to allow users to control the table, just like column width. Let's summarize these, considering ultimate goal of providing more user control and flexibility.

Given these pros and cons, a hybrid approach could be considered, where the grid defaults to auto-fit row heights but allows users to override this with custom height settings. This approach would aim to balance ease of use with flexibility. Let me just try some implementations on Browser-Managed auto-fit and get a brief idea of how much efforts we need and test its reliability. Will comment more here.

kgcreative commented 8 months ago

A potential enhancement here would be to allow the configuration to specify the max height, (ideally in lines of text instead of pixel height). This would allow us to calc a max-height value that could be based in the font line height or some other calculation, but that woudln't require a ton of JS compute, and instead we'd use declarative properties for that. This would also mean that short lines wouldn't have a to of empty space, since we'd still be using CSS/browser rendering, so we'd get the benefit of short lines shrinking to fit.

ashwin-pc commented 8 months ago

So the underlying virtualization library used for data grid is react-window which does not support auto row heights but does support a callback for variable row heights, but the number of lines has to be specified. We also have a requirement coming to make the data grid also support expanding rows, which is another thing that react-window does not support. I'm wondering if we should solve both limitations at the same time.

As for max height, we can add that requirement in too. But it looks like we need to do a modest rewrite of the data grid to support all these features. @BSFishy @ananzh what do you think?

AMoo-Miki commented 8 months ago

Based on the markup that this component builds, I cannot support spending even a minute on it; we will be chasing issues for the next decade with this. I believe we should document the features we want and build something using technologies from this millennium.

BSFishy commented 8 months ago

I agree with Miki, all of the position: absolute has really bothered me since I found out about it. I was under the impression that the component just got out of its incubation period when we inherited it, but the more I learn about it, the more it seems half-baked. +1 on rewriting