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

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

Open
ananzh opened this issue Jan 24, 2024 · 6 comments
Open
Assignees
Labels

Comments

@ananzh
Copy link
Member

ananzh commented Jan 24, 2024

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

  • Utilize the updated RowHeightUtils to calculate row heights on-the-fly.
    Implement logic to recompute row heights when content changes or the grid is resized.

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

BSFishy commented Jan 25, 2024

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

ananzh commented Jan 26, 2024

@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.

  • Default Behavior for Row Heights:
    • In OuiDataGrid, if rowHeightsOptions is not provided, it should default to an auto-fit behavior. This means rows will automatically adjust their height based on their content.
    • The auto-fit feature can be implemented using CSS properties that allow content to define the height naturally, such as height: auto; for the rows and white-space: normal; for the cells to allow wrapping.
  • Modifications to Components:
    • OuiDataGrid: This component doesn't necessarily need to change for implementing auto-fit row heights. It's already set up to pass rowHeightsOptions down to OuiDataGridBody.
    • OuiDataGridBody: The defaultHeight and getRowHeight functions need to be modified so that they can handle the case when rowHeightsOptions is not provided, which would signify the auto-fit feature should be enabled.
    • Cell: In Cell, if rowHeightsOptions is not provided or if getRowHeight returns 'auto', we should ensure that no fixed height styles are applied to the cells so that their height can grow with content. I think we could either change style (use the getRowHeight function to conditionally set the height style.) or add a class to the cells when rowHeightsOptions is not provided and define the necessary CSS to allow the cell heights to fit content.
    • OuiDataGridCell and OuiDataGridCellContent: These components might need updates to ensure that they don't apply any styles or classes that would restrict the natural height of the content. They should be styled to allow content to flow and define the height of the cell.

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.

  • Browser-Managed auto-fit

    • Pros
      • Simplicity: It simplifies the implementation as the browser natively adjusts row heights based on content.
      • Performance: Potentially improves performance by reducing the JavaScript overhead involved in calculating row heights.
    • Cons:
      • Limited Control: It offers less control over the appearance of the grid, as row heights are purely content-driven.
      • Inconsistency: Not consistent with customization designs (not consist with column resize).
      • Over-reliance on Content: For cells with minimal content, the row height might be smaller/bigger than desired, affecting the UX.
  • Calculated Heights auto-fit

    • Pros
      • Flexibility: Offers high flexibility, allowing users to define specific row heights, either globally or per row.
      • Consistency: Ensures a uniform appearance across the grid, which can be crucial for certain types of data presentation.
      • Customization: Aligns with the goal of offering a fully customizable grid experience, including user-controlled row heights.
    • Cons
      • Complexity: Increases the complexity of the grid's codebase, requiring more sophisticated logic to manage and calculate row heights.
      • Performance Impact: Potentially impacts performance, especially for large grids, due to the overhead of calculating heights.
      • User Burden: Places the burden of managing row heights on the user, which might not always be ideal, especially for non-technical users.

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

kgcreative commented Feb 1, 2024

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

ashwin-pc commented Feb 1, 2024

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

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

BSFishy commented Feb 1, 2024

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

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

No branches or pull requests

5 participants