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

[EuiBasicTable][EuiInMemoryTable] Enable more action props to accept an optional callback + fix missing tooltips on collapsed actions #7373

Merged
merged 11 commits into from
Nov 20, 2023

Conversation

cee-chen
Copy link
Contributor

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

Summary

@alvarezmelissa87 is working on a table that needs conditional action descriptions. She was previously attempting to use custom rendered actions to achieve this, but it would be much cleaner all around for their use case for EUI to simply allow actions.description to be a callback which we pass the current item to.

I also added this functionality to the href and data-test-subj keys while here, because why not/it seems useful! Example usage:

<EuiBasicTable
  columns={[
    // ... columns
   {
      name: 'Actions',
      actions: [{
        name: (item) => `Delete ${item.name}`,
        description: (item) => item.someCondition ? 'This does X' : 'This does Y',
        href: (item) => `/delete/${item.id}`,
        'data-test-subj': (item) => `delete-${item.id}`,
      }],
    },
  ]}
/>

Actions tooltip bugfix

While here, I noticed that the action descriptions (which normally renders as a delayed tooltip) was not correctly showing in the collapsed popover menu when they should be, so I added that in d29b623:

actions

Doing this required adding a new toolTipsProps prop to EuiContextMenu so we could pass delay="long" to match the other actions tooltips (ba28dc0)

As always, please review by commit (because I found a few more bugs even more bugs after that 🙈)

QA

  • Go to https://eui.elastic.co/pr_7373/#/tabular-content/tables#adding-actions-to-table
  • Hover or focus on a non-disabled link action (online rows are enabled, offline rows are disabled)
  • Confirm the action description tooltip shows the row name
  • Toggle the "Multiple Actions" switch in the top left corner
  • Confirm that keyboard tabbing into the actions column correctly makes the action buttons visible, unlike production
  • When clicking the "All actions" button, confirm that actions in the popover menu have tooltips, unlike production

General checklist

  • Browser QA
    - [ ] Checked in both light and dark modes
    - [ ] Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • 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

- convert tests to RTL, and remove unnecessary extra snapshot

- remove TS comment, and replace extends with `,` workaround
+ `collapsed_item_actions` is repeating a util already in there unnecessarily, DRY it out too
@cee-chen cee-chen force-pushed the table-actions-continued branch 3 times, most recently from 17c0dd7 to fae65fa Compare November 17, 2023 04:58
+ silence `act()` errors caused by the fact that we load multiple versions of testing-library/DOM (tests still pass otherwise)
…actions

- this involves adding `toolTipProps` to `EuiContextMenuItem`

- deprecate a few other top-level tooltip props while here, for simpler API usage
+ update actions example to include props table
- was broken for `<a>` links but not `<button>`s, doh
@cee-chen cee-chen force-pushed the table-actions-continued branch from fae65fa to 32bce13 Compare November 17, 2023 05:06
@cee-chen cee-chen marked this pull request as ready for review November 17, 2023 05:07
@cee-chen cee-chen requested a review from a team as a code owner November 17, 2023 05:07
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I left a couple comments about Safari + VO not honoring tooltips anymore, but that appears to be a change / bug introduced by Apple.

Comment on lines +88 to +91
ariaLabelledBy = (
<EuiScreenReaderOnly>
<span id={ariaLabelId}>{actionContent}</span>
</EuiScreenReaderOnly>
Copy link
Contributor

Choose a reason for hiding this comment

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

This will go a long way to ensuring screen reader users have context when tooltips appear. Users will know exactly what their next keypress will do in terms of adding / editing / deleting.

Copy link
Contributor

Choose a reason for hiding this comment

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

VoiceOver on Safari has changed the way it announces and handles aria-describedby and I haven't found much to explain why. This combination doesn't announce the tooltip text at all, though VoiceOver + Firefox does. Windows screen readers announce the tooltips properly.

The only thing I could find was this issue ticket filed last year that references the WCAG samples not working correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this as well when I was testing tooltips on Safari+VO! But I found the same behavior on the EuiToolTip page as well so I concluded it wasn't my changes causing it 😬 I do worry a bit that maybe something we did on EUI caused it, let me try to dig into it a bit more separately from this PR

@cee-chen cee-chen merged commit ada8592 into elastic:main Nov 20, 2023
8 checks passed
@cee-chen cee-chen deleted the table-actions-continued branch November 20, 2023 21:34
@cee-chen cee-chen mentioned this pull request Nov 29, 2023
35 tasks
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
Development

Successfully merging this pull request may close these issues.

4 participants