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

Add tag bulk action context menu #82816

Merged
merged 26 commits into from
Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3106167
add the delete tag bulk action
pgayvallet Nov 6, 2020
c35af85
add unit tests for bulk delete
pgayvallet Nov 6, 2020
d93a19a
fix duplicate i18n key
pgayvallet Nov 6, 2020
e6e50f3
add RBAC test on bulk delete
pgayvallet Nov 6, 2020
068c769
add functional tests
pgayvallet Nov 9, 2020
74944f8
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 9, 2020
ad7b509
self review
pgayvallet Nov 9, 2020
bdb270d
design nits
pgayvallet Nov 9, 2020
a9989df
add maxWidth option for confirm modal and add missing doc
pgayvallet Nov 9, 2020
d05f01c
change bulk delete confirm modal max width
pgayvallet Nov 9, 2020
32391bb
add more missing doc
pgayvallet Nov 9, 2020
17ff9d1
only show loading state when performing the bulk delete
pgayvallet Nov 9, 2020
eebe986
use spacer instead of custom margin on horizontal rule
pgayvallet Nov 10, 2020
1452ccc
use link instead of button to remove custom styles
pgayvallet Nov 10, 2020
cc5bcc7
remove spacers, just use styles
pgayvallet Nov 10, 2020
aea10e6
add divider when action menu is displayed
pgayvallet Nov 10, 2020
d68c4a3
set max-width for single delete confirm
pgayvallet Nov 10, 2020
7c5c9d5
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 11, 2020
c3f3a74
a11y fixes
Nov 11, 2020
71236d5
Merge pull request #2 from myasonik/a11y-fixes-for-tags
pgayvallet Nov 12, 2020
58d68fa
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 12, 2020
3529160
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 16, 2020
02ab502
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 17, 2020
8e1ce3d
address nits
pgayvallet Nov 17, 2020
f03823d
Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…
pgayvallet Nov 18, 2020
acc1cfd
add aria-label to delete action
pgayvallet Nov 18, 2020
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
11 changes: 11 additions & 0 deletions x-pack/plugins/saved_objects_tagging/common/test_utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { SavedObject, SavedObjectReference } from 'src/core/types';
import { Tag, TagAttributes } from '../types';
import { TagsCapabilities } from '../capabilities';

export const createTagReference = (id: string): SavedObjectReference => ({
type: 'tag',
Expand Down Expand Up @@ -35,3 +36,13 @@ export const createTagAttributes = (parts: Partial<TagAttributes> = {}): TagAttr
color: '#FF00CC',
...parts,
});

export const createTagCapabilities = (parts: Partial<TagsCapabilities> = {}): TagsCapabilities => ({
view: true,
create: true,
edit: true,
delete: true,
assign: true,
viewConnections: true,
...parts,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import {
overlayServiceMock,
notificationServiceMock,
} from '../../../../../../src/core/public/mocks';
import { tagClientMock } from '../../tags/tags_client.mock';
import { TagBulkAction } from '../types';
import { getBulkDeleteAction } from './bulk_delete';

describe('bulkDeleteAction', () => {
let tagClient: ReturnType<typeof tagClientMock.create>;
let overlays: ReturnType<typeof overlayServiceMock.createStartContract>;
let notifications: ReturnType<typeof notificationServiceMock.createStartContract>;
let action: TagBulkAction;

const tagIds = ['id-1', 'id-2', 'id-3'];

beforeEach(() => {
tagClient = tagClientMock.create();
overlays = overlayServiceMock.createStartContract();
notifications = notificationServiceMock.createStartContract();

action = getBulkDeleteAction({ tagClient, overlays, notifications });
});

it('performs the operation if the confirmation is accepted', async () => {
overlays.openConfirm.mockResolvedValue(true);

await action.execute(tagIds);

expect(overlays.openConfirm).toHaveBeenCalledTimes(1);

expect(tagClient.bulkDelete).toHaveBeenCalledTimes(1);
expect(tagClient.bulkDelete).toHaveBeenCalledWith(tagIds);

expect(notifications.toasts.addSuccess).toHaveBeenCalledTimes(1);
});

it('does not perform the operation if the confirmation is rejected', async () => {
overlays.openConfirm.mockResolvedValue(false);

await action.execute(tagIds);

expect(overlays.openConfirm).toHaveBeenCalledTimes(1);

expect(tagClient.bulkDelete).not.toHaveBeenCalled();
expect(notifications.toasts.addSuccess).not.toHaveBeenCalled();
});

it('does not show notification if `client.bulkDelete` rejects ', async () => {
overlays.openConfirm.mockResolvedValue(true);
tagClient.bulkDelete.mockRejectedValue(new Error('error calling bulkDelete'));

await expect(action.execute(tagIds)).rejects.toMatchInlineSnapshot(
`[Error: error calling bulkDelete]`
);

expect(notifications.toasts.addSuccess).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { OverlayStart, NotificationsStart } from 'src/core/public';
import { i18n } from '@kbn/i18n';
import { ITagInternalClient } from '../../tags';
import { TagBulkAction } from '../types';

interface GetBulkDeleteActionOptions {
overlays: OverlayStart;
notifications: NotificationsStart;
tagClient: ITagInternalClient;
}

export const getBulkDeleteAction = ({
overlays,
notifications,
tagClient,
}: GetBulkDeleteActionOptions): TagBulkAction => {
return {
id: 'delete',
label: i18n.translate('xpack.savedObjectsTagging.management.actions.bulkDelete.label', {
defaultMessage: 'Delete',
}),
Comment on lines +27 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like adding a count here would require rearchitecting a little so I didn't dig too much into it. Is adding a count something we could feasibly do? (It's not super critical but would be nice)

Suggested change
label: i18n.translate('xpack.savedObjectsTagging.management.actions.bulkDelete.label', {
defaultMessage: 'Delete',
}),
label: i18n.translate('xpack.savedObjectsTagging.management.actions.bulkDelete.label', {
defaultMessage: 'Delete',
}),
'aria-label': i18n.translate('xpack.savedObjectsTagging.management.actions.bulkDelete.label', {
defaultMessage: 'Delete {count} tags',
values: { count },
}),

Copy link
Contributor Author

@pgayvallet pgayvallet Nov 12, 2020

Choose a reason for hiding this comment

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

Would require that aria-label is a function passing the number of currently selected tag. Should not be that complicated.

Copy link
Contributor Author

@pgayvallet pgayvallet Nov 12, 2020

Choose a reason for hiding this comment

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

Would an aria-label such as Delete selected tags be enough though? It would avoid changing the action construction from within the action bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Delete selected tags is better than Delete but less good than Delete {count} tags

icon: 'trash',
refreshAfterExecute: true,
execute: async (tagIds) => {
const confirmed = await overlays.openConfirm(
myasonik marked this conversation as resolved.
Show resolved Hide resolved
i18n.translate('xpack.savedObjectsTagging.management.actions.bulkDelete.confirm.text', {
defaultMessage:
'By deleting {count, plural, one {this tag} other {these tags}}, you will no longer be able to assign {count, plural, one {it} other {them}} to saved objects. ' +
'{count, plural, one {This tag} other {These tags}} will be removed from any saved objects that currently use {count, plural, one {it} other {them}}. ' +
'Are you sure you wish to proceed?',
values: {
count: tagIds.length,
},
}),
{
title: i18n.translate(
'xpack.savedObjectsTagging.management.actions.bulkDelete.confirm.title',
{
defaultMessage: 'Delete {count, plural, one {1 tag} other {# tags}}',
values: {
count: tagIds.length,
},
}
),
confirmButtonText: i18n.translate(
'xpack.savedObjectsTagging.management.actions.bulkDelete.confirm.confirmButtonText',
{
defaultMessage: 'Delete {count, plural, one {tag} other {tags}}',
values: {
count: tagIds.length,
},
}
),
buttonColor: 'danger',
Comment on lines +59 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to delete button text and color, would it be possible to also specify an icon (trash) to precede the button text, as shown in the design? This would also apply to the non-bulk, individual delete tag modals as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not right now unfortunately. We had the same issue in the MVP for the single 'delete' action. I created #81482 at this time.

}
);

if (confirmed) {
await tagClient.bulkDelete(tagIds);

notifications.toasts.addSuccess({
title: i18n.translate(
'xpack.savedObjectsTagging.management.actions.bulkDelete.notification.successTitle',
{
defaultMessage: 'Deleted {count, plural, one {1 tag} other {# tags}}',
values: {
count: tagIds.length,
},
}
),
});
}
},
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';
import { TagBulkAction } from '../types';

interface GetClearSelectionActionOptions {
clearSelection: () => void;
}

export const getClearSelectionAction = ({
clearSelection,
}: GetClearSelectionActionOptions): TagBulkAction => {
return {
id: 'clear_selection',
label: i18n.translate('xpack.savedObjectsTagging.management.actions.clearSelection.label', {
defaultMessage: 'Clear selection',
}),
icon: 'cross',
refreshAfterExecute: true,
execute: async () => {
clearSelection();
},
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { coreMock } from '../../../../../../src/core/public/mocks';
import { createTagCapabilities } from '../../../common/test_utils';
import { TagsCapabilities } from '../../../common/capabilities';
import { tagClientMock } from '../../tags/tags_client.mock';
import { TagBulkAction } from '../types';

import { getBulkActions } from './index';

describe('getBulkActions', () => {
let core: ReturnType<typeof coreMock.createStart>;
let tagClient: ReturnType<typeof tagClientMock.create>;
let clearSelection: jest.Mock;

beforeEach(() => {
core = coreMock.createStart();
tagClient = tagClientMock.create();
clearSelection = jest.fn();
});

const getActions = (caps: Partial<TagsCapabilities>) =>
getBulkActions({
core,
tagClient,
clearSelection,
capabilities: createTagCapabilities(caps),
});

const getIds = (actions: TagBulkAction[]) => actions.map((action) => action.id);

it('only returns the `delete` action if user got `delete` permission', () => {
let actions = getActions({ delete: true });

expect(getIds(actions)).toContain('delete');

actions = getActions({ delete: false });

expect(getIds(actions)).not.toContain('delete');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { CoreStart } from 'src/core/public';
import { TagsCapabilities } from '../../../common';
import { ITagInternalClient } from '../../tags';
import { TagBulkAction } from '../types';
import { getBulkDeleteAction } from './bulk_delete';
import { getClearSelectionAction } from './clear_selection';

interface GetBulkActionOptions {
core: CoreStart;
capabilities: TagsCapabilities;
tagClient: ITagInternalClient;
clearSelection: () => void;
}

export const getBulkActions = ({
core: { notifications, overlays },
capabilities,
tagClient,
clearSelection,
}: GetBulkActionOptions): TagBulkAction[] => {
const actions: TagBulkAction[] = [];

if (capabilities.delete) {
actions.push(getBulkDeleteAction({ notifications, overlays, tagClient }));
}

// only add clear selection if user has permission to perform any other action
// as having at least one action will show the bulk action menu, and the selection column on the table
// and we want to avoid doing that only for the 'unselect' action.
if (actions.length > 0) {
actions.push(getClearSelectionAction({ clearSelection }));
}

return actions;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.tagMgt__actionBar {
> .euiHorizontalRule {
margin: 8px 0 3px;
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}
}
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved

.tagMgt__actionBar + .euiSpacer {
display: none;
}
Comment on lines +1 to +3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

elastic/eui#4103 unblocked us by allowing to inject arbitrary content between the InMemoryTable's searchbar and table. However the EuiSpace added when using childrenBetween conflicts with the expected design, so I have to hack it into display: none;

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this whole file goes against our policies for custom styles on top of EUI. There are a couple of issues:

  1. Don't target .eui classes. All our component accept a className prop which you can use to create your own class instance and append to the EUI component class list. Then target this class name so that, in case EUI ever changes it's naming structure (most likely to happen within the next year), your styles won't break.
  2. Don't use hard-coded pixel values. Always use our SASS variables for sizing and font sizes. Most likely a prop exists on the component to help this as well.

If you're having trouble affecting change on an EUI component, please let us know before targeting the .eui classes directly. Most likely we can get a fix in pretty quickly, or if there's a long-term solution being worked on, for instances like these, it's better to be consistent with the EUI components than to override them.

Copy link
Contributor Author

@pgayvallet pgayvallet Nov 10, 2020

Choose a reason for hiding this comment

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

Don't target .eui classes. All our component accept a className

Unfortunately I can't here. I'm targeting the EuiSpacer that is inside the EuiInMemoryTable, between the childrenBetween node and the table. See elastic/eui#4103 / https://github.com/elastic/eui/blob/master/src/components/basic_table/in_memory_table.tsx#L695.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Mostly my comment for that specifically is to say, please ask us or create a PR on the EUI side so we can support your needs without needing hacks. I see this as a valid request that we shouldn't really be introducing a hard-coded spacer here and leave it up to the consumer to provide one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pgayvallet ! I've already got a PR up for you to fix this elastic/eui#4246

I'll let it slide for now 😉 because at least when the fix merges into Kibana, your hack won't have any ill effect. I'd def then appreciate a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #83161. Will do it as soon as elastic/eui#4246 get shipped in next Kibana EUI update.

Thanks again for the quick fix btw


// $euiBorderColor
Loading