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

[EuiDataGrid] Cell actions redesign #7343

Merged
merged 22 commits into from
Nov 20, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 5, 2023

Summary

This PR redesigns the cell actions UI/UX to sit outside the cell instead of inside (which resolves several issues with actions clipping content), and additionally repositions the cell popover to sit on top of the cell instead of next to it or below it.

⚠️ Worth noting: this change enabled us to remove an extra DOM wrapper within the <EuiDataGridCell>, specifically the .euiDataGridRowCell__contentWrapper node. While this is good news in terms of relatively cleaner HTML/CSS, it may also cause breakages to datagrid CSS overrides in Kibana.

Before screencaps

Hover state Focus state Cell popover

After screencaps

Hover state Focus state Cell popover

Cell popover extending to larger column width (see #5686):

popover_large

Code review

This was a lot of incredibly technical CSS changes, so as always, I strongly recommend following along by commit 😬

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile (DEV NOTE: The small cell actions are a bit of a pain to tap on phones, but IMO this is not worse than it already was in production)
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A, no APIs/props changed
  • Code quality checklist
  • 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
    • Updated the Figma library counterpart

@cee-chen cee-chen force-pushed the datagrid/cell-actions branch 3 times, most recently from e5e1606 to 95e65cf Compare November 8, 2023 00:08
@cee-chen cee-chen force-pushed the datagrid/cell-actions branch 8 times, most recently from 6c84194 to 5acfb5c Compare November 16, 2023 00:00
@cee-chen cee-chen marked this pull request as ready for review November 16, 2023 00:32
@cee-chen cee-chen requested a review from a team as a code owner November 16, 2023 00:32
@cee-chen
Copy link
Member Author

@MichaelMarcialis - do you mind QAing https://eui.elastic.co/pr_7343/#/tabular-content/data-grid and ensuring that everything looks and feels up to your expectations for the new design?

Also, do you mind updating the Data Grid screen in EUI's Figma library? 🙏

@cee-chen
Copy link
Member Author

@jughosta since you're also working on EuiDataGrid updates at the same time, how do you feel about reviewing this PR on behalf of the EUI team? 😄 I'm also thinking we should create a new feature branch for these datagrid updates to try and get them all in for the same release. WDYT?

@cee-chen cee-chen changed the base branch from main to datagrid-redesigns November 16, 2023 19:13
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

Code changes look great! I manually tested cell actions on desktop and mobile and confirmed they work as expected.

…t-size to `__content`

- the content wrapper was only needed for flex alignment of cells, and is no longer needed if we start absolutely positioning cell actions

+ move height CSS modifier class to content
- not longer needed due to removed wrapper / placement of padding in DOM
+ clarify types docs to note that new icons are far more restrictive in terms of display
+ remove delay on hover - IMO, no need for this at this point since the cell actions do not occlude cell content
+ flash of content on initial load for cell contents being limited by the new inline max-width
- `.realClick` causes issues on cell actions now due to the new transform animation, switch to `.click`

- not totally what's going on with scrolling realMount, but oh well
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

Comment on lines -88 to -89
max-width: 400px !important;
max-height: 400px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing these styles might be also a breaking change

Copy link
Member Author

@cee-chen cee-chen Nov 20, 2023

Choose a reason for hiding this comment

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

Ah thanks for flagging that Julia! FWIW they're not totally removed, they instead now live as inline styles here:

panelStyle={{
maxInlineSize: `min(75vw, max(${
popoverAnchor.parentElement!.offsetWidth
}px, 400px))`,
maxBlockSize: '50vh',
}}

They needed to be moved to JS since the max width is now semi-dynamic based on cell/column width.

If necessary consumers can still override this via !important to the same .euiDataGridRowCell__popover className.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, @cee-chen. It looks great! I left two very small comments, but otherwise this looks good-to-go from my perspective. Approving now so I don't hold you up further.

src/components/datagrid/_data_grid_data_row.scss Outdated Show resolved Hide resolved
&:hover:not(:focus, :focus-within, .euiDataGridRowCell--open) {
.euiDataGridRowCell__actions {
// Delay the actions showing on hover
// (Note: the focus ring show instantly on hover & the actions show instantly on focus)
animation-delay: $euiAnimSpeedNormal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be open to changing the hover delay for the action buttons to $euiAnimSpeedSlow. The delay looks great. Just hoping to have it be a bit longer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally!

Copy link
Member Author

Choose a reason for hiding this comment

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

d3992f5 - let me know if you think this is enough! We do have ExtraSlow as an option lol

…actions are visible vs not visible

- use a pseudo element to fill the visual gap instead, which respects the animation/hover/display state

- going to dry out the color to a CSS variable here shortly
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen
Copy link
Member Author

@MichaelMarcialis I'm going to go ahead and merge this in, but it'll be to a feature branch - please feel free to leave another comment or ping me on Slack if you want to continue extending the animation delay!

@cee-chen cee-chen merged commit 7430ef5 into elastic:datagrid-redesigns Nov 20, 2023
6 checks passed
@cee-chen cee-chen deleted the datagrid/cell-actions branch November 20, 2023 22:21
@kertal
Copy link
Member

kertal commented Nov 22, 2023

@cee-chen it's great that this was fixed! thx!

cee-chen added a commit to elastic/kibana that referenced this pull request Jan 5, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([#7392](elastic/eui#7392))

## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0)

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([elastic#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([elastic#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([elastic#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([elastic#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([elastic#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([elastic#7392](elastic/eui#7392))

## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0)

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([elastic#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([elastic#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([elastic#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([elastic#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([elastic#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([elastic#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([elastic#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([elastic#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([elastic#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([elastic#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([elastic#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([elastic#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([elastic#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([elastic#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([elastic#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([elastic#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([elastic#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([elastic#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([elastic#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([elastic#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([elastic#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([elastic#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([elastic#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([elastic#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([elastic#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([elastic#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants