[EuiDataGrid] Enforce a cell popover if cell actions are present (#5710)

* [misc setup] refactor cell actions unit tests to use mock component - this allows us to `component.find()` by the 'MockAction' name, which future unit tests will be using to determine the # of actions rendered * Misc docs cleanup - Add intro paragraph to cell popovers page, explaining the UX purpose of the expansion popover - Fix incorrect cellActions example in main datagrid doc - Fix variable casing on source code * Force cell isExpandable to evaluate as true if cellActions exist + add more comments/documentation to note this behavior since I missed it the first time + simplify hasCellActions to just use isExpandable, now that isExpandable accounts for cellActions * Add changelog entry
elastic · Mar 17, 2022 · 7d640c0 · 7d640c0
1 parent b4d6a28
commit 7d640c0
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 55 deletions.
diff --git a/src-docs/src/views/datagrid/_snippets.tsx b/src-docs/src/views/datagrid/_snippets.tsx
@@ -15,16 +15,12 @@ inMemory={{ level: 'sorting' }}`,
     actions: { showMoveLeft: false, showMoveRight: false }, // doesn't show move actions in column header
     schema: 'franchise', // custom schema later defined under schemaDetectors
     cellActions: [ // provides one additional cell action that triggers an alert once clicked
-      {
-        label: 'test',
-        iconType: 'heart',
-        callback: ()=> alert('test')
-      }
-    ]
-  }
+      ({ Component }) => <Component iconType="heart" onClick={() => alert('test')}>Custom action</Component>,
+    ],
+  },
 ]}`,
   columnVisibility: `columnVisibility={{
-  visibleColumns: ['A', 'C'],
+  visibleColumns: ['A'],
   setVisibleColumns: () => {},
 }}`,
   leadingControlColumns: `leadingControlColumns={[
@@ -61,7 +57,7 @@ inMemory={{ level: 'sorting' }}`,
   onChangeItemsPerPage: () => {},
 }}`,
   sorting: `sorting={{
-  columns: [{ id: 'C', direction: 'asc' }],
+  columns: [{ id: 'A', direction: 'asc' }],
   onSort: () => {},
 }}`,
   toolbarVisibility: `toolbarVisibility={{

diff --git a/src-docs/src/views/datagrid/cells_popovers/cell_popover_is_expandable.tsx b/src-docs/src/views/datagrid/cells_popovers/cell_popover_is_expandable.tsx
@@ -28,7 +28,7 @@ const columns: EuiDataGridColumn[] = [
   },
   {
     id: 'lastName',
-    isExpandable: false,
+    isExpandable: false, // Overridden by the fact that cellActions is set
     cellActions,
   },
   {

diff --git a/src-docs/src/views/datagrid/cells_popovers/datagrid_cell_popover_example.js b/src-docs/src/views/datagrid/cells_popovers/datagrid_cell_popover_example.js
@@ -4,7 +4,7 @@ import { GuideSectionTypes } from '../../../components';
 import { EuiCode, EuiCallOut, EuiSpacer } from '../../../../../src';
 
 import IsDetailsPopover from './cell_popover_is_details';
-const IsDetailsPopoverSource = require('!!raw-loader!./cell_popover_is_details');
+const isDetailsPopoverSource = require('!!raw-loader!./cell_popover_is_details');
 
 import RenderCellPopover from './cell_popover_rendercellpopover';
 const renderCellPopoverSource = require('!!raw-loader!./cell_popover_rendercellpopover');
@@ -19,7 +19,6 @@ import {
 } from '!!prop-loader!../../../../../src/components/datagrid/data_grid_types';
 
 export const DataGridCellPopoverExample = {
-  title: 'Data grid cell popovers',
   sections: [
     {
       title: 'Conditionally customizing cell popover content',
@@ -48,7 +47,7 @@ export const DataGridCellPopoverExample = {
       source: [
         {
           type: GuideSectionTypes.TSX,
-          code: IsDetailsPopoverSource,
+          code: isDetailsPopoverSource,
         },
       ],
       props: {
@@ -137,10 +136,10 @@ export const DataGridCellPopoverExample = {
       text: (
         <>
           <p>
-            Popovers can sometimes be unnecessary for short form content. In the
-            example below we&apos;ve turned them off by setting{' '}
-            <EuiCode>isExpandable=false</EuiCode> on specific{' '}
-            <EuiCode>columns</EuiCode>.
+            Popovers can sometimes be unnecessary for short form content, and
+            can be disabled by setting <EuiCode>columns.isExpandable</EuiCode>{' '}
+            to <EuiCode>false</EuiCode>. In the example below, we&apos;ve turned
+            off expansion on the suffix column.
           </p>
           <p>
             To set <EuiCode>isExpandable</EuiCode> at a per-cell level instead
@@ -149,6 +148,16 @@ export const DataGridCellPopoverExample = {
             example conditionally disables the expansion popover for boolean
             cells that are &apos;false&apos;.
           </p>
+          <EuiCallOut
+            color="warning"
+            iconType="alert"
+            title="Cells with actions are always expandable"
+          >
+            If <EuiCode>columns.cellActions</EuiCode> is defined,{' '}
+            <EuiCode>isExpandable</EuiCode> will always be forced to true. This
+            ensures that keyboard and screen reader users have access to all
+            cell actions.
+          </EuiCallOut>
         </>
       ),
       demo: <IsExpandablePopover />,

diff --git a/src-docs/src/views/datagrid/cells_popovers/datagrid_cells_example.js b/src-docs/src/views/datagrid/cells_popovers/datagrid_cells_example.js
@@ -1,4 +1,5 @@
 import React, { Fragment } from 'react';
+import { Link } from 'react-router-dom';
 
 import { GuideSectionTypes } from '../../../components';
 import {
@@ -7,6 +8,7 @@ import {
   EuiCallOut,
   EuiBasicTable,
   EuiSpacer,
+  EuiText,
   EuiListGroupItem,
 } from '../../../../../src';
 
@@ -27,6 +29,28 @@ import {
 
 export const DataGridCellsExample = {
   title: 'Data grid cells & popovers',
+  intro: (
+    <EuiText>
+      <p>
+        Data grid cells are rendered using the{' '}
+        <EuiCode>renderCellValue</EuiCode> prop. This prop accepts a function
+        which is treated as a React component behind the scenes. This means the
+        data grid passes cell-specific props (e.g. row index, column/schema
+        info, etc.) to your render function, which can ouput anything from a
+        string to any JSX element.
+      </p>
+      <p>
+        Each data grid cell will automatically render an expand action button
+        and an expansion popover (
+        <Link to="#disabling-cell-expansion-popovers">
+          which can be disabled
+        </Link>
+        ). For cells with overflowing or truncated content, this expansion
+        popover helps display the full amount of content, or can be customized
+        to show extra details.
+      </p>
+    </EuiText>
+  ),
   sections: [
     {
       source: [

diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx
@@ -391,6 +391,18 @@ describe('EuiDataGridCell', () => {
   });
 
   describe('isExpandable', () => {
+    it('always returns true if column.cellActions exists', () => {
+      const component = mount(
+        <EuiDataGridCell
+          {...requiredProps}
+          column={{ id: 'someId', cellActions: [() => <button />] }}
+          isExpandable={false}
+        />
+      );
+
+      expect(component.find('renderCellValue').prop('isExpandable')).toBe(true);
+    });
+
     it('falls back to props.isExpandable which is derived from the column config', () => {
       const component = mount(
         <EuiDataGridCell {...requiredProps} isExpandable={true} />

diff --git a/src/components/datagrid/body/data_grid_cell.tsx b/src/components/datagrid/body/data_grid_cell.tsx
@@ -429,6 +429,10 @@ export class EuiDataGridCell extends Component<
   };
 
   isExpandable = () => {
+    // A cell must always show an expansion popover if it has cell actions,
+    // otherwise keyboard and screen reader users have no way of accessing them
+    if (this.props.column?.cellActions?.length) return true;
+
     // props.isExpandable inherits from column.isExpandable
     // state.cellProps allows consuming applications to override isExpandable on a per-cell basis
     return this.state.cellProps.isExpandable ?? this.props.isExpandable;
@@ -516,7 +520,6 @@ export class EuiDataGridCell extends Component<
 
     const isExpandable = this.isExpandable();
     const popoverIsOpen = this.isPopoverOpen();
-    const hasCellActions = isExpandable || column?.cellActions;
     const showCellActions =
       this.state.isFocused ||
       this.state.isEntered ||
@@ -652,7 +655,7 @@ export class EuiDataGridCell extends Component<
       </EuiFocusTrap>
     );
 
-    if (hasCellActions) {
+    if (isExpandable) {
       innerContent = (
         <div className={anchorClass} ref={this.popoverAnchorRef}>
           <div className={expandClass}>
@@ -663,7 +666,6 @@ export class EuiDataGridCell extends Component<
               rowIndex={rowIndex}
               colIndex={colIndex}
               column={column}
-              isExpandable={isExpandable}
               closePopover={closeCellPopover}
               onExpandClick={() => {
                 if (popoverIsOpen) {

diff --git a/src/components/datagrid/body/data_grid_cell_actions.test.tsx b/src/components/datagrid/body/data_grid_cell_actions.test.tsx
@@ -9,11 +9,16 @@
 import React from 'react';
 import { shallow } from 'enzyme';
 
+import { EuiDataGridColumnCellAction } from '../data_grid_types';
 import {
   EuiDataGridCellActions,
   EuiDataGridCellPopoverActions,
 } from './data_grid_cell_actions';
 
+const MockAction: EuiDataGridColumnCellAction = ({ Component }) => (
+  <Component iconType="starEmpty" data-test-subj="mockCellAction" />
+);
+
 describe('EuiDataGridCellActions', () => {
   const requiredProps = {
     closePopover: jest.fn(),
@@ -23,9 +28,7 @@ describe('EuiDataGridCellActions', () => {
   };
 
   it('renders an expand button', () => {
-    const component = shallow(
-      <EuiDataGridCellActions {...requiredProps} isExpandable={true} />
-    );
+    const component = shallow(<EuiDataGridCellActions {...requiredProps} />);
 
     expect(component).toMatchInlineSnapshot(`
       <div
@@ -61,27 +64,10 @@ describe('EuiDataGridCellActions', () => {
     const component = shallow(
       <EuiDataGridCellActions
         {...requiredProps}
-        isExpandable={false}
-        column={{ id: 'someId', cellActions: [() => <button />] }}
+        column={{ id: 'someId', cellActions: [MockAction] }}
       />
     );
 
-    expect(component).toMatchInlineSnapshot(`
-      <div
-        className="euiDataGridRowCell__expandActions"
-      >
-        <Component
-          Component={[Function]}
-          closePopover={[MockFunction]}
-          colIndex={0}
-          columnId="someId"
-          isExpanded={false}
-          key="0"
-          rowIndex={0}
-        />
-      </div>
-    `);
-
     const button = component.childAt(0).renderProp('Component');
     expect(button({ iconType: 'eye' })).toMatchInlineSnapshot(`
       <EuiButtonIcon
@@ -97,16 +83,15 @@ describe('EuiDataGridCellActions', () => {
     const component = shallow(
       <EuiDataGridCellActions
         {...requiredProps}
-        isExpandable={true}
-        column={{ id: 'someId', cellActions: [() => <button />] }}
+        column={{ id: 'someId', cellActions: [MockAction] }}
       />
     );
 
     expect(component).toMatchInlineSnapshot(`
       <div
         className="euiDataGridRowCell__expandActions"
       >
-        <Component
+        <MockAction
           Component={[Function]}
           closePopover={[MockFunction]}
           colIndex={0}
@@ -133,7 +118,7 @@ describe('EuiDataGridCellPopoverActions', () => {
       <EuiDataGridCellPopoverActions
         colIndex={0}
         rowIndex={0}
-        column={{ id: 'someId', cellActions: [() => <button />] }}
+        column={{ id: 'someId', cellActions: [MockAction] }}
       />
     );
 
@@ -147,7 +132,7 @@ describe('EuiDataGridCellPopoverActions', () => {
           <EuiFlexItem
             key="0"
           >
-            <Component
+            <MockAction
               Component={[Function]}
               colIndex={0}
               columnId="someId"
@@ -159,11 +144,8 @@ describe('EuiDataGridCellPopoverActions', () => {
       </EuiPopoverFooter>
     `);
 
-    const button = component
-      .childAt(0)
-      .childAt(0)
-      .childAt(0) // .find('Component') doesn't work, for whatever reason
-      .renderProp('Component');
+    const action = component.find('MockAction') as any;
+    const button = action.renderProp('Component');
     expect(button({ iconType: 'function' })).toMatchInlineSnapshot(`
       <EuiButtonEmpty
         iconType="function"

diff --git a/src/components/datagrid/body/data_grid_cell_actions.tsx b/src/components/datagrid/body/data_grid_cell_actions.tsx
@@ -20,21 +20,23 @@ import { EuiFlexGroup, EuiFlexItem } from '../../flex';
 import { EuiPopoverFooter } from '../../popover';
 
 export const EuiDataGridCellActions = ({
-  isExpandable,
   closePopover,
   onExpandClick,
   column,
   rowIndex,
   colIndex,
 }: {
-  isExpandable: boolean;
   closePopover: () => void;
   onExpandClick: () => void;
   column?: EuiDataGridColumn;
   rowIndex: number;
   colIndex: number;
 }) => {
-  const expandButton = isExpandable ? (
+  // Note: The cell expand button/expansion popover is *always* rendered if
+  // column.cellActions is present (regardless of column.isExpandable).
+  // This is because cell actions are not otherwise accessible to keyboard
+  // or screen reader users
+  const expandButton = (
     <EuiI18n
       key={'expand'}
       token="euiDataGridCellActions.expandButtonTitle"
@@ -54,7 +56,7 @@ export const EuiDataGridCellActions = ({
         />
       )}
     </EuiI18n>
-  ) : null;
+  );
 
   const additionalButtons = useMemo(() => {
     const ButtonComponent = (props: EuiButtonIconProps) => (

diff --git a/upcoming_changelogs/5710.md b/upcoming_changelogs/5710.md
@@ -0,0 +1 @@
+- `EuiDataGrid` now forces `isExpandable` to be true if any `cellActions` are passed, as keyboard users are otherwise unable to access cell actions without the expansion popover
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- `EuiDataGrid` now forces `isExpandable` to be true if any `cellActions` are passed, as keyboard users are otherwise unable to access cell actions without the expansion popover