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] Fix regressed disabled actions UX when selecting rows #7428

Merged
merged 5 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/upcoming/7428.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as disabled when rows are being selected
5 changes: 4 additions & 1 deletion src-docs/src/views/tables/actions/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,10 @@ export default () => {

return (
<>
<EuiFlexGroup alignItems="center">
<EuiFlexGroup
alignItems="center"
css={({ euiTheme }) => ({ minHeight: euiTheme.size.xxl })}
>
<EuiFlexItem grow={false}>
<EuiSwitch
label="Multiple Actions"
Expand Down
5 changes: 5 additions & 0 deletions src-docs/src/views/tables/actions/actions_section.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ export const section = {
When more than 2 actions are supplied, only the ellipses icon button
stays visible at all times.
</li>
<li>
When one or more table row(s) are selected, all item actions are
disabled. Users should be expected to use some bulk action outside the
individual table rows instead.
</li>
</ul>
</>
),
Expand Down
152 changes: 100 additions & 52 deletions src/components/basic_table/basic_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ import {
} from './basic_table';

import { SortDirection } from '../../services';
import { EuiTableFieldDataColumnType } from './table_types';
import {
EuiTableFieldDataColumnType,
EuiTableActionsColumnType,
} from './table_types';

describe('getItemId', () => {
it('returns undefined if no itemId prop is given', () => {
Expand Down Expand Up @@ -635,27 +638,65 @@ describe('EuiBasicTable', () => {
});

describe('actions', () => {
const actions: EuiTableActionsColumnType<{ id: string }>['actions'] = [
{
type: 'icon',
name: 'Edit',
isPrimary: true,
icon: 'pencil',
available: ({ id }) => !(Number(id) % 2),
description: 'edit',
'data-test-subj': 'editAction',
onClick: () => {},
},
{
type: 'icon',
name: 'Share',
icon: 'share',
isPrimary: true,
available: ({ id }) => id !== '3',
description: 'share',
onClick: () => {},
},
// Below actions are not primary and should be hidden behind collapse button
{
type: 'icon',
name: 'Copy',
icon: 'copy',
description: 'copy',
onClick: () => {},
},
{
type: 'icon',
name: 'Delete',
icon: 'trash',
description: 'delete',
'data-test-subj': ({ id }) => `deleteAction-${id}`,
onClick: () => {},
},
{
type: 'icon',
name: 'elastic.co',
icon: 'link',
description: 'Go to link',
onClick: () => {},
},
];

test('single action', () => {
const props: EuiBasicTableProps<BasicItem> = {
items: basicItems,
columns: [
...basicColumns,
{
name: 'Actions',
actions: [
{
type: 'button',
name: 'Edit',
description: 'edit',
onClick: () => {},
},
],
actions: [actions[3]],
},
],
};
const { getAllByText } = render(<EuiBasicTable {...props} />);

expect(getAllByText('Edit')).toHaveLength(basicItems.length);
expect(getAllByText('Delete')).toHaveLength(basicItems.length);
});

test('multiple actions with custom availability', () => {
Expand All @@ -665,48 +706,7 @@ describe('EuiBasicTable', () => {
...basicColumns,
{
name: 'Actions',
actions: [
{
type: 'icon',
name: 'Edit',
isPrimary: true,
icon: 'pencil',
available: ({ id }) => !(Number(id) % 2),
description: 'edit',
onClick: () => {},
},
{
type: 'icon',
name: 'Share',
icon: 'share',
isPrimary: true,
available: ({ id }) => id !== '3',
description: 'share',
onClick: () => {},
},
// Below actions are not primary and should be hidden behind collapse button
{
type: 'icon',
name: 'Copy',
icon: 'copy',
description: 'copy',
onClick: () => {},
},
{
type: 'icon',
name: 'Delete',
icon: 'trash',
description: 'delete',
onClick: () => {},
},
{
type: 'icon',
name: 'elastic.co',
icon: 'link',
description: 'Go to link',
onClick: () => {},
},
],
actions: actions,
},
],
};
Expand All @@ -720,6 +720,54 @@ describe('EuiBasicTable', () => {
4
);
});

describe('are disabled on selection', () => {
test('single action', () => {
const props: EuiBasicTableProps<BasicItem> = {
items: basicItems,
selection: {
onSelectionChange: () => {},
selected: [basicItems[0]],
},
columns: [
...basicColumns,
{
name: 'Actions',
actions: [actions[3]],
},
],
};
const { getByTestSubject } = render(<EuiBasicTable {...props} />);

expect(getByTestSubject('deleteAction-1')).toBeDisabled();
expect(getByTestSubject('deleteAction-2')).toBeDisabled();
expect(getByTestSubject('deleteAction-3')).toBeDisabled();
});

test('multiple actions', () => {
const props: EuiBasicTableProps<BasicItem> = {
items: basicItems,
selection: {
onSelectionChange: () => {},
selected: [basicItems[0]],
},
columns: [
...basicColumns,
{
name: 'Actions',
actions: actions,
},
],
};
const { getAllByTestSubject } = render(<EuiBasicTable {...props} />);

getAllByTestSubject('euiCollapsedItemActionsButton').forEach(
(button) => {
expect(button).toBeDisabled();
}
);
});
});
});

it('renders (kitchen sink) with pagination, selection, sorting, actions, and footer', () => {
Expand Down
20 changes: 12 additions & 8 deletions src/components/basic_table/basic_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1153,17 +1153,21 @@ export class EuiBasicTable<T = any> extends Component<
column: EuiTableActionsColumnType<T>,
columnIndex: number
) {
const actionEnabled = (action: Action<T>) =>
this.state.selection.length === 0 &&
(!action.enabled || action.enabled(item));
// Disable all actions if any row(s) are selected
const allDisabled = this.state.selection.length > 0;

let actualActions = column.actions.filter(
(action: Action<T>) => !action.available || action.available(item)
);
if (actualActions.length > 2) {
// if any of the actions `isPrimary`, add them inline as well, but only the first 2
const primaryActions = actualActions.filter((o) => o.isPrimary);
actualActions = primaryActions.slice(0, 2);
if (allDisabled) {
// If all actions are disabled, do not show any actions but the popover toggle
actualActions = [];
} else {
// if any of the actions `isPrimary`, add them inline as well, but only the first 2
const primaryActions = actualActions.filter((o) => o.isPrimary);
actualActions = primaryActions.slice(0, 2);
}

// if we have more than 1 action, we don't show them all in the cell, instead we
// put them all in a popover tool. This effectively means we can only have a maximum
Expand All @@ -1177,9 +1181,9 @@ export class EuiBasicTable<T = any> extends Component<
return (
<CollapsedItemActions
actions={column.actions}
actionsDisabled={allDisabled}
itemId={itemId}
item={item}
actionEnabled={actionEnabled}
/>
);
},
Expand All @@ -1189,9 +1193,9 @@ export class EuiBasicTable<T = any> extends Component<
const tools = (
<ExpandedItemActions
actions={actualActions}
actionsDisabled={allDisabled}
itemId={itemId}
item={item}
actionEnabled={actionEnabled}
/>
);

Expand Down
6 changes: 3 additions & 3 deletions src/components/basic_table/collapsed_item_actions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('CollapsedItemActions', () => {
],
itemId: 'id',
item: { id: '1' },
actionEnabled: () => true,
actionsDisabled: false,
};

const { container } = render(<CollapsedItemActions {...props} />);
Expand All @@ -63,7 +63,7 @@ describe('CollapsedItemActions', () => {
],
itemId: 'id',
item: { id: 'xyz' },
actionEnabled: () => true,
actionsDisabled: false,
};

const { getByTestSubject, getByText, baseElement } = render(
Expand Down Expand Up @@ -92,7 +92,7 @@ describe('CollapsedItemActions', () => {
],
itemId: 'id',
item: { id: 'xyz' },
actionEnabled: () => true,
actionsDisabled: false,
};

const { getByTestSubject, baseElement } = render(
Expand Down
32 changes: 20 additions & 12 deletions src/components/basic_table/collapsed_item_actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,18 @@ export interface CollapsedItemActionsProps<T extends {}> {
actions: Array<Action<T>>;
item: T;
itemId: ItemIdResolved;
actionEnabled: (action: Action<T>) => boolean;
actionsDisabled: boolean;
className?: string;
}

export const CollapsedItemActions = <T extends {}>({
actions,
itemId,
item,
actionEnabled,
actionsDisabled,
className,
}: CollapsedItemActionsProps<T>) => {
const [popoverOpen, setPopoverOpen] = useState(false);
const [allDisabled, setAllDisabled] = useState(true);

const onClickItem = useCallback((onClickAction?: () => void) => {
setPopoverOpen(false);
Expand All @@ -57,8 +56,7 @@ export const CollapsedItemActions = <T extends {}>({
const available = action.available?.(item) ?? true;
if (!available) return controls;

const enabled = actionEnabled(action);
if (enabled) setAllDisabled(false);
const enabled = action.enabled == null || action.enabled(item);

if (isCustomItemAction<T>(action)) {
const customAction = action as CustomItemAction<T>;
Expand Down Expand Up @@ -94,7 +92,7 @@ export const CollapsedItemActions = <T extends {}>({
<EuiContextMenuItem
key={index}
className="euiBasicTable__collapsedAction"
disabled={!enabled}
disabled={!enabled && !actionsDisabled}
href={href}
target={target}
icon={icon}
Expand All @@ -111,25 +109,35 @@ export const CollapsedItemActions = <T extends {}>({
}
return controls;
}, []);
}, [actions, actionEnabled, item, onClickItem]);
}, [actions, actionsDisabled, item, onClickItem]);

const popoverButton = (
<EuiI18n token="euiCollapsedItemActions.allActions" default="All actions">
{(allActions: string) => (
<EuiI18n
tokens={[
'euiCollapsedItemActions.allActions',
'euiCollapsedItemActions.allActionsDisabled',
]}
defaults={[
'All actions',
'Individual item actions are disabled when rows are being selected.',
]}
>
{([allActions, allActionsDisabled]: string[]) => (
<EuiButtonIcon
className={className}
aria-label={allActions}
aria-label={actionsDisabled ? allActionsDisabled : allActions}
title={actionsDisabled ? allActionsDisabled : undefined}
iconType="boxesHorizontal"
color="text"
isDisabled={allDisabled}
isDisabled={actionsDisabled}
onClick={() => setPopoverOpen((isOpen) => !isOpen)}
data-test-subj="euiCollapsedItemActionsButton"
/>
)}
</EuiI18n>
);

const withTooltip = !allDisabled && (
const withTooltip = !actionsDisabled && (
<EuiI18n token="euiCollapsedItemActions.allActions" default="All actions">
{(allActions: ReactNode) => (
<EuiToolTip content={allActions} delay="long">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('ExpandedItemActions', () => {
],
itemId: 'xyz',
item: { id: 'xyz' },
actionEnabled: () => true,
actionsDisabled: false,
};

const component = shallow(<ExpandedItemActions {...props} />);
Expand Down
Loading
Loading