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

enhance grouping for context menu options #3924

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multi DataSource] UX enhancement on Data source management stack ([#2521](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2521))
- [Multi DataSource] UX enhancement on Update stored password modal for Data source management stack ([#2532](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2532))
- [Monaco editor] Add json worker support ([#3424](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3424))
- Enhance grouping for context menus ([#3169](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3169))

### 🐛 Bug Fixes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export const ContextMenuExamples: React.FC = () => {
<p>
Below examples show how context menu panels look with varying number of actions and how the
actions can be grouped into different panels using <EuiCode>grouping</EuiCode> field.
Grouping can only be one layer deep. A group needs to have at least two items for grouping
to work. A separator is automatically added between groups.
</p>

<EuiFlexGroup>
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/ui_actions/public/actions/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export interface Action<Context extends BaseContext = {}, T = ActionType>
* Returns a title to be displayed to the user.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it helps, I can change this comment to inform users that a React element is also supported here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that'd be helpful.

* @param context
*/
getDisplayName(context: ActionExecutionContext<Context>): string;
getDisplayName(context: ActionExecutionContext<Context>): JSX.Element | string;

/**
* `UiComponent` to render when displaying this action as a context menu item.
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/ui_actions/public/actions/action_internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class ActionInternal<A extends ActionDefinition = ActionDefinition>
return this.definition.getIconType(context);
}

public getDisplayName(context: Context<A>): string {
public getDisplayName(context: Context<A>): JSX.Element | string {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this since Display name is misleading here? getDisplayElement perhaps?

Copy link
Contributor Author

@sikhote sikhote May 15, 2023

Choose a reason for hiding this comment

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

Agreed that name would be better, but I'd recommend against it at this point, because it would mean a lot of downstream changes as well (such as other plugins not in this repository).

if (!this.definition.getDisplayName) return `Action: ${this.id}`;
return this.definition.getDisplayName(context);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,28 @@ const createTestAction = ({
type,
dispayName,
order,
grouping,
}: {
type?: string;
dispayName: string;
order?: number;
grouping?: any[];
}) =>
createAction({
type: type as any, // mapping doesn't matter for this test
getDisplayName: () => dispayName,
order,
execute: async () => {},
grouping,
});

const resultMapper = (panel: EuiContextMenuPanelDescriptor) => ({
items: panel.items ? panel.items.map((item) => ({ name: item.name })) : [],
items: panel.items
? panel.items.map((item) => ({
...(item.name ? { name: item.name } : {}),
...(item.isSeparator ? { isSeparator: true } : {}),
}))
: [],
});

test('sorts items in DESC order by "order" field first, then by display name', async () => {
Expand Down Expand Up @@ -248,3 +256,195 @@ test('hides items behind in "More" submenu if there are more than 4 actions', as
]
`);
});

test('flattening of group with only one action', async () => {
const grouping1 = [
{
id: 'test-group',
getDisplayName: () => 'Test group',
getIconType: () => 'bell',
},
];
const actions = [
createTestAction({
dispayName: 'Foo 1',
}),
createTestAction({
dispayName: 'Bar 1',
grouping: grouping1,
}),
];
const menu = await buildContextMenuForActions({
actions: actions.map((action) => ({ action, context: {}, trigger: 'TEST' as any })),
});

expect(menu.map(resultMapper)).toMatchInlineSnapshot(`
Array [
Object {
"items": Array [
Object {
"name": "Foo 1",
},
Object {
"isSeparator": true,
},
Object {
"name": "Bar 1",
},
],
},
Object {
"items": Array [
Object {
"name": "Bar 1",
},
],
},
]
`);
});

test('grouping with only two actions', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

thanks for all the tests!

const grouping1 = [
{
id: 'test-group',
getDisplayName: () => 'Test group',
getIconType: () => 'bell',
},
];
const actions = [
createTestAction({
dispayName: 'Foo 1',
}),
createTestAction({
dispayName: 'Bar 1',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Bar 2',
grouping: grouping1,
}),
];
const menu = await buildContextMenuForActions({
actions: actions.map((action) => ({ action, context: {}, trigger: 'TEST' as any })),
});

expect(menu.map(resultMapper)).toMatchInlineSnapshot(`
Array [
Object {
"items": Array [
Object {
"name": "Foo 1",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group",
},
],
},
Object {
"items": Array [
Object {
"name": "Bar 1",
},
Object {
"name": "Bar 2",
},
],
},
]
`);
});

test('groups with deep nesting', async () => {
const grouping1 = [
{
id: 'test-group',
getDisplayName: () => 'Test group',
getIconType: () => 'bell',
},
];
const grouping2 = [
{
id: 'test-group-2',
getDisplayName: () => 'Test group 2',
getIconType: () => 'bell',
},
{
id: 'test-group-3',
getDisplayName: () => 'Test group 3',
getIconType: () => 'bell',
},
];

const actions = [
createTestAction({
dispayName: 'Foo 1',
}),
createTestAction({
dispayName: 'Bar 1',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Bar 2',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Qux 1',
grouping: grouping2,
}),
];
const menu = await buildContextMenuForActions({
actions: actions.map((action) => ({ action, context: {}, trigger: 'TEST' as any })),
});

expect(menu.map(resultMapper)).toMatchInlineSnapshot(`
Array [
Object {
"items": Array [
Object {
"name": "Foo 1",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group 3",
},
],
},
Object {
"items": Array [
Object {
"name": "Bar 1",
},
Object {
"name": "Bar 2",
},
],
},
Object {
"items": Array [
Object {
"name": "Test group 3",
},
],
},
Object {
"items": Array [
Object {
"name": "Qux 1",
},
],
},
]
`);
});
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export async function buildContextMenuForActions({
closeMenu = () => {},
}: BuildContextMenuParams): Promise<EuiContextMenuPanelDescriptor[]> {
const panels: Record<string, PanelDescriptor> = {
// This is the first panel which links out to all others via items property
mainMenu: {
id: 'mainMenu',
title,
Expand All @@ -157,35 +158,51 @@ export async function buildContextMenuForActions({
const context: ActionExecutionContext<object> = { ...item.context, trigger: item.trigger };
const isCompatible = await item.action.isCompatible(context);
if (!isCompatible) return;
let parentPanel = '';
let currentPanel = '';

// Reference to the last/parent/upper group.
// Groups are provided in order of parent to children.
let parentGroupId = '';

if (action.grouping) {
for (let i = 0; i < action.grouping.length; i++) {
const group = action.grouping[i];
currentPanel = group.id;
if (!panels[currentPanel]) {
const groupId = group.id;

// If a panel does not exist for the current group, then create it
if (!panels[groupId]) {
const name = group.getDisplayName ? group.getDisplayName(context) : group.id;
panels[currentPanel] = {
id: currentPanel,

// Create panel for group
panels[groupId] = {
id: groupId,
title: name,
items: [],
_level: i,
_icon: group.getIconType ? group.getIconType(context) : 'empty',
};
if (parentPanel) {
panels[parentPanel].items!.push({

// If there are multiple groups and this is not the first group,
// then add an item to the parent group relating to this group
if (parentGroupId) {
panels[parentGroupId].items!.push({
name,
panel: currentPanel,
panel: groupId,
icon: group.getIconType ? group.getIconType(context) : 'empty',
_order: group.order || 0,
_title: group.getDisplayName ? group.getDisplayName(context) : '',
});
}
}
parentPanel = currentPanel;

// Save the current group, because it will be used as the parent group
// for adding items to it for any additional groups in the array
parentGroupId = groupId;
}
}
panels[parentPanel || 'mainMenu'].items!.push({

// Add a context menu item for this action so it shows up on a context menu panel.
// We add this within the parent group or default to the mainMenu panel.
panels[parentGroupId || 'mainMenu'].items!.push({
name: action.MenuItem
? React.createElement(uiToReactComponent(action.MenuItem), { context })
: action.getDisplayName(context),
Expand All @@ -197,8 +214,10 @@ export async function buildContextMenuForActions({
_title: action.getDisplayName(context),
});
});

await Promise.all(promises);

// For each panel, sort items by order and title
for (const panel of Object.values(panels)) {
const items = panel.items.filter(Boolean) as ItemDescriptor[];
panel.items = _.sortBy(
Expand All @@ -208,13 +227,23 @@ export async function buildContextMenuForActions({
);
}

// On the mainMenu, before adding in items for other groups, the first 4 items are shown.
// Any additional items are hidden behind a "more" item
wrapMainPanelItemsIntoSubmenu(panels, 'mainMenu');

for (const panel of Object.values(panels)) {
// If the panel is a root-level panel, such as the parent of a group,
// then create mainMenu item for this panel
if (panel._level === 0) {
// TODO: Add separator line here once it is available in EUI.
// See https://github.com/elastic/eui/pull/4018
if (panel.items.length > 3) {
// Add separator with unique key if needed
if (panels.mainMenu.items.length) {
panels.mainMenu.items.push({ isSeparator: true, key: `${panel.id}separator` });
}

// If a panel has more than one child, then allow items to be grouped
// and link to it in the mainMenu. Otherwise, flatten the group.
// Note: this only happens on the root level panels, not for inner groups.
if (panel.items.length > 1) {
panels.mainMenu.items.push({
name: panel.title || panel.id,
icon: panel._icon || 'empty',
Expand Down