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
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export interface EmbeddableFactory<
* Returns a display name for this type of embeddable. Used in "Create new... " options
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.

Do we need this change? This is for the Create new button for embeddables right?

* in the add panel for containers.
*/
getDisplayName(): string;
getDisplayName(): JSX.Element | string;

/**
* If false, this type of embeddable can't be created with the "createNew" functionality. Instead,
Expand Down
13 changes: 12 additions & 1 deletion src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ interface Props {
SavedObjectFinder: React.ComponentType<any>;
stateTransfer?: EmbeddableStateTransfer;
hideHeader?: boolean;
// By default, embeddable.destroy() is called when this component unmounts.
// To prevent this default behavior, set this prop to true.
isDestroyPrevented?: boolean;
// Toggle off the border and shadow applied around the embeddable.
// By default, the embeddable will have a shadow and border around it.
isBorderless?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Hey @sikhote, why are these two changes necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the process of checking creating an example of this in the Examples section, I discovered there is an existing EmbeddableRenderer that does the job. These changes are now removed.

}

interface State {
Expand Down Expand Up @@ -200,7 +206,10 @@ export class EmbeddablePanel extends React.Component<Props, State> {
if (this.state.errorEmbeddable) {
this.state.errorEmbeddable.destroy();
}
this.props.embeddable.destroy();

if (!this.props.isDestroyPrevented) {
this.props.embeddable.destroy();
}
}

public onFocus = (focusedPanelIndex: string) => {
Expand Down Expand Up @@ -234,6 +243,8 @@ export class EmbeddablePanel extends React.Component<Props, State> {
paddingSize="none"
role="figure"
aria-labelledby={headerId}
hasBorder={!this.props.isBorderless}
hasShadow={!this.props.isBorderless}
>
{!this.props.hideHeader && (
<PanelHeader
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,132 @@ test('hides items behind in "More" submenu if there are more than 4 actions', as
]
`);
});

test('tests groups and separators', async () => {
const grouping1 = [
{
id: 'test-group',
getDisplayName: () => 'Test group',
getIconType: () => 'bell',
},
];
const grouping2 = [
{
id: 'test-group-2',
getDisplayName: () => 'Test group 2',
getIconType: () => 'bell',
},
];
const grouping3 = [
{
id: 'test-group-3',
getDisplayName: () => 'Test group 3',
getIconType: () => 'bell',
},
{
id: 'test-group-4',
getDisplayName: () => 'Test group 4',
getIconType: () => 'bell',
},
];

const actions = [
createTestAction({
dispayName: 'First action',
}),
createTestAction({
dispayName: 'Foo 1',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Foo 2',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Foo 3',
grouping: grouping1,
}),
createTestAction({
dispayName: 'Bar 1',
grouping: grouping2,
}),
createTestAction({
dispayName: 'Bar 2',
grouping: grouping2,
}),
createTestAction({
dispayName: 'Inner',
grouping: grouping3,
}),
];
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": "First action",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group 2",
},
Object {
"isSeparator": true,
},
Object {
"name": "Test group 4",
},
],
},
Object {
"items": Array [
Object {
"name": "Foo 1",
},
Object {
"name": "Foo 2",
},
Object {
"name": "Foo 3",
},
],
},
Object {
"items": Array [
Object {
"name": "Bar 1",
},
Object {
"name": "Bar 2",
},
],
},
Object {
"items": Array [
Object {
"name": "Test group 4",
},
],
},
Object {
"items": Array [
Object {
"name": "Inner",
},
],
},
]
`);
});
Original file line number Diff line number Diff line change
Expand Up @@ -157,35 +157,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 group...groups are provided in array order of
// parent to children (or outer to inner)
let parentPanelId = '';

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 (!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 panel relating to this group
if (parentPanelId) {
panels[parentPanelId].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 panel, because this will be used for adding items
// to it from later groups in the array
parentPanelId = groupId;
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure, nothing functionally chnages here right? You are just renaming currentPannel to groupId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, renaming and adding comments for future users.

}
}
panels[parentPanel || 'mainMenu'].items!.push({

// If grouping exists, parentPanelId will be the most-inner group,
// otherwise use the mainMenu panel.
// Add item for action to this most-inner panel or mainMenu.
panels[parentPanelId || 'mainMenu'].items!.push({
name: action.MenuItem
? React.createElement(uiToReactComponent(action.MenuItem), { context })
: action.getDisplayName(context),
Expand All @@ -197,6 +213,7 @@ export async function buildContextMenuForActions({
_title: action.getDisplayName(context),
});
});

await Promise.all(promises);

for (const panel of Object.values(panels)) {
Expand All @@ -211,10 +228,17 @@ export async function buildContextMenuForActions({
wrapMainPanelItemsIntoSubmenu(panels, 'mainMenu');

for (const panel of Object.values(panels)) {
// If the panel is a root-level panel, such as a group-based panel,
// 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 item's to be grouped
// and link to it in the mainMenu. Otherwise, flatten the group.
if (panel.items.length > 1) {
panels.mainMenu.items.push({
name: panel.title || panel.id,
icon: panel._icon || 'empty',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/ui_actions/public/util/presentable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export interface Presentable<Context extends object = object> {
/**
* 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.

Same here for changing this comment to inform users that a React element is also supported here.

*/
getDisplayName(context: Context): string;
getDisplayName(context: Context): JSX.Element | string;

/**
* Returns tooltip text which should be displayed when user hovers this object.
Expand Down