[EuiDataGrid] Various row height fixes #8025

cee-chen · 2024-09-14T09:11:46Z

Summary

addreses [EuiDataGrid] Cell text content visible below truncation when using fontSize: "s" #7780 (comment)
closes Data grid checkboxes are misaligned with row height #7897
closes [EuiDataGrid] Ensure gridStyles updates trigger a cell height recalculation #7811

This PR contains several fixes as well as storybook/VRT test updates. I strongly recommend following along by commit, as I continued to update VRT screenshots as I fixed bugs, which is helpful in seeing the visual diffs.

QA

Go to https://eui.elastic.co/pr_8025/storybook/?path=/story/tabular-content-euidatagrid-rowheightsoptions-prop--custom-line-height
Change the line height from 40px to, e.g. 5
Confirm row heights update accordingly
Click the display settings selector in the datagrid toolbar and switch the row height to Custom (2 lines)
Confirm that the control column checkbox and action buttons remain aligned with the first line of text, and do not have alignment issues/move when toggled between Single and Custom
Go to https://eui.elastic.co/pr_8025/storybook/?path=/story/tabular-content-euidatagrid-gridstyle-prop--playground
Toggle the various fontSize and cellPadding option permutations and confirm that row height(s) update accordingly/as expected
Go to https://eui.elastic.co/pr_8025/storybook/?path=/story/tabular-content-euidatagrid-rowheightsoptions-prop--custom-row-heights
Confirm the last row is truncated correctly at 2 lines, and its text is not incorrectly clipped
Click the display settings selector in the datagrid toolbar and switch the density to Compact and the row height to Custom with 1 line per row
Confirm that the Version badges are visible until the end of the cell and are not cut off by the compact padding, i.e.

General checklist

Browser QA
- Checked in Chrome, Safari, Edge, and Firefox
- Checked in mobile
  ~~- [ ] Checked in both light and dark modes~~
  ~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
Docs site QA - N/A
Code quality checklist
- Added or updated jest and cypress tests
- Updated visual regression tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

…duction usages - including complex children such as checkboxes & badges

- content is no longer inside a button, so text align is no longer needed and makes CSS overrides a little easier

… single to multi row lines

- we're only rendering 5 rows so index 5 wasn't doing anything / applying to any row. this also clearly reveals a bug with lineCount: 2 - `onChange` needs to be reset to undefined because of `enableFunctionToggleControls`+`controls.include`

😭 not sure what past-me 3 years ago was smoking, but the `excludePadding` prop is no longer needed

…ons.lineHeight` change - requires waterfalling `gridStyles` down to EuiDataGridCell

- remove the cached styles and just use `window.getComputedStyles` - which causes a reflow, but since we need it for lineHeight in any case we might as well keep using it 🤷

packages/eui/src/components/datagrid/data_grid.stories.utils.tsx

mgadewoll · 2024-09-18T14:02:40Z

packages/eui/src/components/datagrid/body/cell/data_grid_cell.styles.ts

      autoHeight: css`
        ${logicalCSS('height', 'auto')}
      `,
      defaultHeight: css`
        ${logicalCSS('height', '100%')}
      `,
+      // Control columns should be vertically centered with the *first line* of text
+      // for both single and multi-line heights (see https://github.com/elastic/eui/issues/7897)
+      controlColumn: css`


Should we align the content horizontally as well?
Currently the controls in the story examples are not centered for cellPadding="s | l"

I noticed this too and I'm debating a separate PR for that where controlColumns.width does not include the cell padding, so that it scales with different densities and this off-centeredness doesn't occur.

Center alignment would solve the issue for the compact density, but not the expanded density, since technically the width provided is too small for both the expanded cell padding + the checkbox width.

Either way I'd like to address that separately from this PR, since that has more to do with column widths and less to do with row heights, and changing how the width API works for control columns would technically be a breaking change.

Thanks for the context! I'm fine with addressing it in a separate PR then 👍

packages/eui/src/components/datagrid/data_grid_row_heights.stories.tsx

packages/eui/src/components/datagrid/body/cell/data_grid_cell.tsx

packages/eui/src/components/datagrid/utils/row_heights.ts

elasticmachine · 2024-09-18T17:29:20Z

💚 Build Succeeded

Buildkite Build
Commit: 1f35ef5

History

💚 Build #2754 succeeded 4ccf3a1
💚 Build #2753 succeeded b8d5c6a
💚 Build #2743 succeeded 52bb4d5

mgadewoll

🚢 🐈‍⬛ Great work fixing all those smaller issues on datagrid! 👏

cee-chen · 2024-09-19T15:41:23Z

Thanks a million as always for the thorough review/QA Lene!

`v95.11.0`⏩`v95.12.0-backport.0` > [!note] > A few fun highlights from this release: > - Content within `EuiDataGrid`, `EuiBasicTable`, and `EuiInMemoryTable`, when manually selected/highlighted by users, should now more cleanly copy and paste into various spreadsheet apps > - `EuiProvider` will now detect the user's system-level dark or light mode and default to that if no `colorMode` prop is passed manually _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v95.12.0`](https://github.com/elastic/eui/releases/v95.12.0) - Enhanced `EuiDataGrid` and `EuiBasic/InMemoryTable` to clean content newlines/tabs when users copy and paste from their tabular data ([#8019](elastic/eui#8019)) - Updated `EuiResizableButton` with a new `accountForScrollbars` prop ([#8021](elastic/eui#8021)) - Updated `EuiProvider` to inherit from the user's OS/system light/dark mode setting if a `colorMode` prop has not been passed ([#8026](elastic/eui#8026)) **Bug fixes** - Fixed `EuiDatePicker`'s `onClear` button to not appear when the input is `disabled` ([#8020](elastic/eui#8020)) - Fixed several `EuiDataGrid` row height bugs: ([#8025](elastic/eui#8025)) - Fixed row heights not recalculating when `rowHeightOptions.lineHeight`, `gridStyles.fontSize`, or `gridStyles.cellPadding` changed - Fixed incorrect height calculations for `rowHeightOptions.rowHeights` with `lineCount`s - Fixed control column content to align better with multi-line row heights, as well as custom line-heights ## [`v95.12.0-backport.0`](https://github.com/elastic/eui/releases/v95.12.0-backport.0) **This is a backport release only intended for use by Kibana.** **Bug fixes** - Fixed `EuiProvider`'s system color mode detection causing errors during server-side rendering ([#8040](elastic/eui#8040)) - Fixed an `EuiDataGrid` rendering bug that was causing bouncing scrollbar issues ([#8041](elastic/eui#8041))

`v95.11.0`⏩`v95.12.0-backport.0` > [!note] > A few fun highlights from this release: > - Content within `EuiDataGrid`, `EuiBasicTable`, and `EuiInMemoryTable`, when manually selected/highlighted by users, should now more cleanly copy and paste into various spreadsheet apps > - `EuiProvider` will now detect the user's system-level dark or light mode and default to that if no `colorMode` prop is passed manually _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v95.12.0`](https://github.com/elastic/eui/releases/v95.12.0) - Enhanced `EuiDataGrid` and `EuiBasic/InMemoryTable` to clean content newlines/tabs when users copy and paste from their tabular data ([elastic#8019](elastic/eui#8019)) - Updated `EuiResizableButton` with a new `accountForScrollbars` prop ([elastic#8021](elastic/eui#8021)) - Updated `EuiProvider` to inherit from the user's OS/system light/dark mode setting if a `colorMode` prop has not been passed ([elastic#8026](elastic/eui#8026)) **Bug fixes** - Fixed `EuiDatePicker`'s `onClear` button to not appear when the input is `disabled` ([elastic#8020](elastic/eui#8020)) - Fixed several `EuiDataGrid` row height bugs: ([elastic#8025](elastic/eui#8025)) - Fixed row heights not recalculating when `rowHeightOptions.lineHeight`, `gridStyles.fontSize`, or `gridStyles.cellPadding` changed - Fixed incorrect height calculations for `rowHeightOptions.rowHeights` with `lineCount`s - Fixed control column content to align better with multi-line row heights, as well as custom line-heights ## [`v95.12.0-backport.0`](https://github.com/elastic/eui/releases/v95.12.0-backport.0) **This is a backport release only intended for use by Kibana.** **Bug fixes** - Fixed `EuiProvider`'s system color mode detection causing errors during server-side rendering ([elastic#8040](elastic/eui#8040)) - Fixed an `EuiDataGrid` rendering bug that was causing bouncing scrollbar issues ([elastic#8041](elastic/eui#8041)) (cherry picked from commit 8a89c85)

cee-chen added the data grid label Sep 14, 2024

cee-chen added 7 commits September 17, 2024 11:56

[storybook] Update default EuiDataGrid args to more closely mimic pro…

e63fb84

…duction usages - including complex children such as checkboxes & badges

Add VRT regression test for bug

5ad3ee5

Fix bug by using the undefined/default height type for lineCount: 1

41d4f77

[cleanup] Remove now-unnecessary EuiDataGridHeaderCells button unset

12964b2

- content is no longer inside a button, so text align is no longer needed and makes CSS overrides a little easier

[7897] Fix control column/checkbox vertical alignment when going from…

735c537

… single to multi row lines

Fix bug with custom row heights lineCount

a08b0f9

😭 not sure what past-me 3 years ago was smoking, but the `excludePadding` prop is no longer needed

cee-chen force-pushed the datagrid/linecount-1 branch from 52bb4d5 to e1a225e Compare September 17, 2024 19:45

cee-chen added 3 commits September 17, 2024 15:19

[7811] Fix row heights not changing on gridStyle or `rowHeightsOpti…

0b9ef8a

…ons.lineHeight` change - requires waterfalling `gridStyles` down to EuiDataGridCell

Fix stale cellPadding updates/calculations

fdde820

- remove the cached styles and just use `window.getComputedStyles` - which causes a reflow, but since we need it for lineHeight in any case we might as well keep using it 🤷

changelog

4ccf3a1

cee-chen force-pushed the datagrid/linecount-1 branch from b8d5c6a to 4ccf3a1 Compare September 17, 2024 22:21

cee-chen marked this pull request as ready for review September 17, 2024 22:24

cee-chen requested a review from a team as a code owner September 17, 2024 22:24

mgadewoll reviewed Sep 18, 2024

View reviewed changes

[PR feedback] Fix rowHeights overrides not triggering a recalculation

1f35ef5

mgadewoll approved these changes Sep 19, 2024

View reviewed changes

cee-chen merged commit 84aa840 into elastic:main Sep 19, 2024
5 checks passed

cee-chen deleted the datagrid/linecount-1 branch September 19, 2024 15:59

This was referenced Sep 23, 2024

[Data grid] Single and Custom (set to 1) cell heights are incongruous #8037

Closed

[One Discover] Add summary column for logs contextual experience elastic/kibana#192567

Merged

cee-chen mentioned this pull request Sep 24, 2024

Upgrade EUI to v95.12.0 elastic/kibana#193805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Various row height fixes #8025

[EuiDataGrid] Various row height fixes #8025

cee-chen commented Sep 14, 2024 •

edited by mgadewoll

Loading

mgadewoll Sep 18, 2024

cee-chen Sep 18, 2024 •

edited

Loading

mgadewoll Sep 19, 2024

elasticmachine commented Sep 18, 2024

mgadewoll left a comment

cee-chen commented Sep 19, 2024

[EuiDataGrid] Various row height fixes #8025

[EuiDataGrid] Various row height fixes #8025

Conversation

cee-chen commented Sep 14, 2024 • edited by mgadewoll Loading

Summary

QA

General checklist

mgadewoll Sep 18, 2024

Choose a reason for hiding this comment

cee-chen Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

mgadewoll Sep 19, 2024

Choose a reason for hiding this comment

elasticmachine commented Sep 18, 2024

💚 Build Succeeded

History

mgadewoll left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 19, 2024

cee-chen commented Sep 14, 2024 •

edited by mgadewoll

Loading

cee-chen Sep 18, 2024 •

edited

Loading