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

[EUI Datagrid] Adds rowHeightOptions property #4853

Merged
merged 35 commits into from
Jul 6, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Jun 3, 2021

Summary

Adds possibility to set default Height for all rows and define initial heights for each row. ALso so that content will be expanded by row height we should provide rowHeightsOptions property for datagrid.

API:

{
  defaultHeight: 40, // height which will be applied for all rows except described in initialHeights. Also can be defined as lineCount
  rowHeights: {
    1: 120, // for row which have index 1 we set 120 pixel
    3: { lineCount: 4 }, // for row which have index 3 we allow to show 3 lines after that we truncate
    4: { height: 40 } // for row which have index 4 we set 40 pixel
  }
}

For example here I set default height for each row = 60 and for second row lineCount = 5

image

Issues which we have:

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: comment at #4853 (comment) supersedes this

Looks like a great PoC, thank you for exploring! I'm kinda surprised how straight forward it was to get to even this point. Things we'll need to consider/answer to move forward:

  • computedStylesForGridCell is called any time EuiDataGrid renders, and outside any lifecycle
  • getNumberFromPx - we'll likely want to try to support non-px values
  • Would be great to avoid the extra div in data_grid_cell , and is there a way to extend browser support of this method beyond chrome/webkit?
  • defaultHeight conflicts with the density setting, though this is probably fine as-is

src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_body.tsx Outdated Show resolved Hide resolved
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think cells should vertically align top, shouldn't they? @chandlerprall
Data_grid_row_height_options_-_Elastic_UI_Framework

@chandlerprall
Copy link
Contributor

chandlerprall commented Jun 9, 2021

Spoke with @elastic/kibana-app, let's change the line height configuration to accept either a pixel value or the desired line count. This should remove the need for computing the style & doing other calculations.

We want to support:

  • rows with no configured height (only choice today)
  • rows with line counts
  • rows with pixel heights - these cells might not be able to provide indication of truncation

For the API, I envision something like:

      rowHeightOptions={{
        defaultHeight: 40, // 40 pixels
        initialHeights: {
          1: { lineCount: 3 }, // 3 lines
          4: 140, // 140 pixels
          5: { height: 80 }, // 80 pixels
          6: { height: '1rem' }, // 1 rem
        },
      }}

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cell actions' appearance/creation shifts content

datagrid_height_truncation.mov

src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_body.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chandlerprall
Not 100% sure of it, but shouldn't also the expansion (+ custom cell actions) be vertical aligned top? Would subjectively look better when there's e.g. a single line text displayed next to a multi line cell

Bildschirmfoto 2021-06-14 um 19 10 38

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@chandlerprall
Copy link
Contributor

Not 100% sure of it, but shouldn't also the expansion (+ custom cell actions) be vertical aligned top? Would subjectively look better when there's e.g. a single line text displayed next to a multi line cell

Yep! I didn't call it out as you already had, and it was part of the our discussion last week.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • datagrid_example.js needs to include EuiDataGridRowHeightsOptions in the Props list:
    • include EuiDataGridRowHeightsOptions in the import on lines 21-35
    • pass EuiDataGridRowHeightsOptions to the props object on lines 365-382

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src-docs/src/views/datagrid/row_height_options.js Outdated Show resolved Hide resolved
src/components/common.ts Outdated Show resolved Hide resolved
src/components/datagrid/row_height_utils.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking this out of draft, asking @miukimiu for a review on the SCSS changes

src/components/datagrid/row_height_utils.ts Outdated Show resolved Hide resolved
@chandlerprall chandlerprall marked this pull request as ready for review June 29, 2021 17:24
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @VladLasitsa for helping with this. It looks great! 🎉

I tested in Chrome, Safari, Edge, and Firefox and it works as expected. I just noticed a few things that are not blockers but you can definitely improve the code/examples.

src/components/datagrid/_data_grid_data_row.scss Outdated Show resolved Hide resolved
src/components/datagrid/data_grid.tsx Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 I pushed a changelog conflict resolution & removed the AtLeastOne utility type from common.ts as it's no longer used. Everything looks good, I'll merge when CI passes.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4853/

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

Successfully merging this pull request may close these issues.

7 participants