[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra `<button>` wrapper + cleanup #7361

cee-chen · 2023-11-14T10:19:55Z

Summary

Flagged in Slack today - on production, both basic and memory tables are incorrectly rendering double wrapping <button>s, which is definitely an axe/a11y violation.

Before	After

⚠️ Please follow along by commit - the first 3 commits are the only required fixes, everything else is tech debt (rewriting tests to RTL, removing some totally unnecessary logic I noticed while here, then converted the files from class to function components because why not).

I also noticed that the mobile UI/UX of custom actions looked pretty meh, and improved that while here (last commit)

Before	After

QA

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 - N/A, bugfix
Code quality checklist - Updated snapshots, but did not add new tests
- Added or updated jest ~~and cypress 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
~~- [ ] Updated the Figma library counterpart~~

…or custom actions

- this causes nesting issues otherwise for consumers that render links/buttons - puttong the onClick close on a wrapper accomplishes the same UX

- for consumers who want to customize the appearance of the popover more, e.g. changing the spacing etc

- convert from enzyme to RTL - snapshot DOM instead of enzyme wrappers - separate tests for custom vs default actions + remove unnecessary onFocus/onBlur fns and actionEnabled arg typing

…tions - it's literally not doing anything - there's no hover or opacity behavior at this point

…nents - now that all the onFocus/onBlur logic is gone, these components can more easily be function components + fix typescript complaints

- attempt to match default actions display while still allowing for custom/wildcard content

1Copenut

👍 I reviewed the Slack thread for context and appreciate the quick turnaround on this fix Cee. Nested interactive elements create a tab stop inside the button that takes focus but doesn't usually get announced by screen readers. Increased cognitive load, rough edge on the UX, both improved with this PR.

changelogs/upcoming/7361.md

kibanamachine · 2023-11-14T19:01:49Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7361/
Storybook: https://eui.elastic.co/pr_7361/storybook

elasticmachine · 2023-11-14T19:01:52Z

💚 Build Succeeded

Buildkite Build
Commit: 5fe0e6a

History

💚 Build #1025 succeeded 40df74f

`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 <julia.rechkunova@elastic.co>

`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 <julia.rechkunova@elastic.co>

cee-chen force-pushed the table-custom-actions branch from 3a8ac38 to d076952 Compare November 14, 2023 10:37

cee-chen added 7 commits November 14, 2023 02:47

[docs] Fix table actions example not rendering the collapsed toggle f…

c5858b1

…or custom actions

Fix custom actions to not render a <button> context menu item

cd5f6ac

- this causes nesting issues otherwise for consumers that render links/buttons - puttong the onClick close on a wrapper accomplishes the same UX

[misc] Add targetable classNames

f5c6554

- for consumers who want to customize the appearance of the popover more, e.g. changing the spacing etc

Improve tests

ea89a13

- convert from enzyme to RTL - snapshot DOM instead of enzyme wrappers - separate tests for custom vs default actions + remove unnecessary onFocus/onBlur fns and actionEnabled arg typing

[cleanup] Remove totally unnecessary focus/blur JS logic on custom ac…

f0b17d9

…tions - it's literally not doing anything - there's no hover or opacity behavior at this point

[optional][tech debt] Convert from class components to function compo…

a5ea254

…nents - now that all the onFocus/onBlur logic is gone, these components can more easily be function components + fix typescript complaints

[enhancement] Improve mobile display of custom actions

40df74f

- attempt to match default actions display while still allowing for custom/wildcard content

cee-chen force-pushed the table-custom-actions branch from d076952 to 40df74f Compare November 14, 2023 10:48

cee-chen marked this pull request as ready for review November 14, 2023 10:50

cee-chen requested a review from a team as a code owner November 14, 2023 10:50

1Copenut approved these changes Nov 14, 2023

View reviewed changes

cee-chen commented Nov 14, 2023

View reviewed changes

changelogs/upcoming/7361.md Outdated Show resolved Hide resolved

changelog typo

5fe0e6a

cee-chen enabled auto-merge (squash) November 14, 2023 18:35

cee-chen merged commit 1b1d980 into elastic:main Nov 14, 2023
6 of 7 checks passed

cee-chen deleted the table-custom-actions branch November 14, 2023 19:01

This was referenced Dec 18, 2023

Upgrade EUI to v91.3.1 elastic/kibana#173569

Merged

[EuiBasicTable] Fix regressed disabled actions UX when selecting rows #7428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra `<button>` wrapper + cleanup #7361

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra `<button>` wrapper + cleanup #7361

cee-chen commented Nov 14, 2023 •

edited

Loading

1Copenut left a comment

kibanamachine commented Nov 14, 2023

elasticmachine commented Nov 14, 2023

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra <button> wrapper + cleanup #7361

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra <button> wrapper + cleanup #7361

Conversation

cee-chen commented Nov 14, 2023 • edited Loading

Summary

QA

General checklist

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 14, 2023

elasticmachine commented Nov 14, 2023

💚 Build Succeeded

History

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra `<button>` wrapper + cleanup #7361

[EuiBasicTable][EuiInMemoryTable] Fix collapsed custom actions rendering an extra `<button>` wrapper + cleanup #7361

cee-chen commented Nov 14, 2023 •

edited

Loading