From ee6507288a7be576e26d075bf95216616c3ca03f Mon Sep 17 00:00:00 2001 From: Constance Chen <constance.chen.3@gmail.com> Date: Tue, 1 Mar 2022 13:16:55 -0800 Subject: [PATCH 1/4] [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 --- .../body/data_grid_cell_actions.test.tsx | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/components/datagrid/body/data_grid_cell_actions.test.tsx b/src/components/datagrid/body/data_grid_cell_actions.test.tsx index eb4056ffe02..2039e3eb448 100644 --- 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(), @@ -62,7 +67,7 @@ describe('EuiDataGridCellActions', () => { <EuiDataGridCellActions {...requiredProps} isExpandable={false} - column={{ id: 'someId', cellActions: [() => <button />] }} + column={{ id: 'someId', cellActions: [MockAction] }} /> ); @@ -70,7 +75,7 @@ describe('EuiDataGridCellActions', () => { <div className="euiDataGridRowCell__expandActions" > - <Component + <MockAction Component={[Function]} closePopover={[MockFunction]} colIndex={0} @@ -98,7 +103,7 @@ describe('EuiDataGridCellActions', () => { <EuiDataGridCellActions {...requiredProps} isExpandable={true} - column={{ id: 'someId', cellActions: [() => <button />] }} + column={{ id: 'someId', cellActions: [MockAction] }} /> ); @@ -106,7 +111,7 @@ describe('EuiDataGridCellActions', () => { <div className="euiDataGridRowCell__expandActions" > - <Component + <MockAction Component={[Function]} closePopover={[MockFunction]} colIndex={0} @@ -133,7 +138,7 @@ describe('EuiDataGridCellPopoverActions', () => { <EuiDataGridCellPopoverActions colIndex={0} rowIndex={0} - column={{ id: 'someId', cellActions: [() => <button />] }} + column={{ id: 'someId', cellActions: [MockAction] }} /> ); @@ -147,7 +152,7 @@ describe('EuiDataGridCellPopoverActions', () => { <EuiFlexItem key="0" > - <Component + <MockAction Component={[Function]} colIndex={0} columnId="someId" @@ -159,11 +164,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" From da8eb04d541d6aef616dc8b92d70fcfa31ae55bd Mon Sep 17 00:00:00 2001 From: Constance Chen <constance.chen.3@gmail.com> Date: Tue, 1 Mar 2022 14:17:38 -0800 Subject: [PATCH 2/4] 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 --- src-docs/src/views/datagrid/_snippets.tsx | 14 ++++------- .../datagrid_cell_popover_example.js | 5 ++-- .../cells_popovers/datagrid_cells_example.js | 24 +++++++++++++++++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src-docs/src/views/datagrid/_snippets.tsx b/src-docs/src/views/datagrid/_snippets.tsx index 428426876f8..fb0d3384e6c 100644 --- 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/datagrid_cell_popover_example.js b/src-docs/src/views/datagrid/cells_popovers/datagrid_cell_popover_example.js index 9d6e6c323ab..dc3a264359c 100644 --- 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: { 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 index b0e2b83a0eb..c5e7bff6f9f 100644 --- 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: [ From 0ca49a8347a3713d4dff24aa85b6d8600adc2e83 Mon Sep 17 00:00:00 2001 From: Constance Chen <constance.chen.3@gmail.com> Date: Tue, 1 Mar 2022 14:14:14 -0800 Subject: [PATCH 3/4] 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 --- .../cell_popover_is_expandable.tsx | 2 +- .../datagrid_cell_popover_example.js | 19 ++++++++++++---- .../datagrid/body/data_grid_cell.test.tsx | 12 ++++++++++ .../datagrid/body/data_grid_cell.tsx | 8 ++++--- .../body/data_grid_cell_actions.test.tsx | 22 +------------------ .../datagrid/body/data_grid_cell_actions.tsx | 10 +++++---- 6 files changed, 40 insertions(+), 33 deletions(-) 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 index 499b457423f..994ed4148f5 100644 --- 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 index dc3a264359c..65bf4acf642 100644 --- 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 @@ -1,4 +1,5 @@ import React from 'react'; +import { Link } from 'react-router-dom'; import { GuideSectionTypes } from '../../../components'; import { EuiCode, EuiCallOut, EuiSpacer } from '../../../../../src'; @@ -136,10 +137,10 @@ export const DataGridCellPopoverExample = { text: ( <> <p> - Popovers can sometimes be unnecessary for short form content. In the - example below we'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've turned + off expansion on the suffix column. </p> <p> To set <EuiCode>isExpandable</EuiCode> at a per-cell level instead @@ -148,6 +149,16 @@ export const DataGridCellPopoverExample = { example conditionally disables the expansion popover for boolean cells that are 'false'. </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/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx index 4355fd8dffc..31ca2e5e20e 100644 --- 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 index a207b63e379..78c6ced31c3 100644 --- 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 index 2039e3eb448..c1e77e8a3a4 100644 --- a/src/components/datagrid/body/data_grid_cell_actions.test.tsx +++ b/src/components/datagrid/body/data_grid_cell_actions.test.tsx @@ -28,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 @@ -66,27 +64,10 @@ describe('EuiDataGridCellActions', () => { const component = shallow( <EuiDataGridCellActions {...requiredProps} - isExpandable={false} column={{ id: 'someId', cellActions: [MockAction] }} /> ); - expect(component).toMatchInlineSnapshot(` - <div - className="euiDataGridRowCell__expandActions" - > - <MockAction - 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 @@ -102,7 +83,6 @@ describe('EuiDataGridCellActions', () => { const component = shallow( <EuiDataGridCellActions {...requiredProps} - isExpandable={true} column={{ id: 'someId', cellActions: [MockAction] }} /> ); diff --git a/src/components/datagrid/body/data_grid_cell_actions.tsx b/src/components/datagrid/body/data_grid_cell_actions.tsx index ff63b402876..cf2649601c4 100644 --- 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) => ( From 63cd8d23ff77e78282cd5b67743711bc68f007cc Mon Sep 17 00:00:00 2001 From: Constance Chen <constance.chen.3@gmail.com> Date: Thu, 10 Mar 2022 10:40:34 -0800 Subject: [PATCH 4/4] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25667bf8d58..4ebb63b0d54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ - Updated `testenv` mock for `EuiIcon` to render `aria-label` as text ([#5709](https://github.com/elastic/eui/pull/5709)) - Added `editorChecklist` glyph to `EuiIcon` ([#5705](https://github.com/elastic/eui/pull/5705)) +- `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 ([#5710](https://github.com/elastic/eui/pull/5710)) **Breaking changes**