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

fix(actions): Better type checks for icons #1496

Merged
merged 2 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@
* under the License.
*/

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { Action } from '../../../../src/plugins/ui_actions/public';

export const sampleAction = (
id: string,
order: number,
name: string,
icon: string,
icon: EuiIconType,
grouping?: Action['grouping']
): Action => {
return {
Expand Down
3 changes: 2 additions & 1 deletion src/core/public/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { Observable } from 'rxjs';
import { History } from 'history';
import { RecursiveReadonly } from '@osd/utility-types';

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { MountPoint } from '../types';
import { Capabilities } from './capabilities';
import { ChromeStart } from '../chrome';
Expand Down Expand Up @@ -190,7 +191,7 @@ export interface App<HistoryLocationState = unknown> {
* A EUI iconType that will be used for the app's icon. This icon
* takes precendence over the `icon` property.
*/
euiIconType?: string;
euiIconType?: EuiIconType;

/**
* A URL to an image file used as an icon. Used as a fallback
Expand Down
3 changes: 2 additions & 1 deletion src/core/public/chrome/nav_links/nav_link.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* under the License.
*/

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { pick } from '@osd/std';
import { AppCategory } from '../../';

Expand Down Expand Up @@ -75,7 +76,7 @@ export interface ChromeNavLink {
* A EUI iconType that will be used for the app's icon. This icon
* takes precedence over the `icon` property.
*/
readonly euiIconType?: string;
readonly euiIconType?: EuiIconType;

/**
* A URL to an image file used as an icon. Used as a fallback
Expand Down
5 changes: 3 additions & 2 deletions src/core/public/chrome/nav_links/nav_links_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { NavLinksService } from './nav_links_service';
import { take, map, takeLast } from 'rxjs/operators';
import { App } from '../../application';
import { BehaviorSubject } from 'rxjs';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';

const availableApps = new Map([
['app1', { id: 'app1', order: 0, title: 'App 1', icon: 'app1' }],
Expand All @@ -41,7 +42,7 @@ const availableApps = new Map([
id: 'app2',
order: -10,
title: 'App 2',
euiIconType: 'canvasApp',
euiIconType: 'canvasApp' as EuiIconType,
Copy link
Member

@ashwin-pc ashwin-pc Apr 26, 2022

Choose a reason for hiding this comment

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

Instead of casting it as EuiIconType can we instead cast it as a const

i.e.

euiIconType: 'canvasApp' as const,

This way we dont go back to the same issue where we use any string and accidentally use a value not available in eui

The same goes for every instance in the PR that you are casting the iconType as EuiIconType

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I forgot that those are really assertions, not casts 😢 . The only one that makes sense to assert is the one test where we are intentionally using a made-up string rather than a valid icon name. Fixed.

},
],
['chromelessApp', { id: 'chromelessApp', order: 20, title: 'Chromless App', chromeless: true }],
Expand Down Expand Up @@ -200,7 +201,7 @@ describe('NavLinksService', () => {
id: 'app2',
order: -10,
title: 'App 2',
euiIconType: 'canvasApp',
euiIconType: 'canvasApp' as EuiIconType,
})
);
const hiddenLinkIds = await start
Expand Down
5 changes: 3 additions & 2 deletions src/core/public/chrome/nav_links/to_nav_link.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { PublicAppInfo, AppNavLinkStatus, AppStatus } from '../../application';
import { toNavLink } from './to_nav_link';

import { httpServiceMock } from '../../mocks';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';

const app = (props: Partial<PublicAppInfo> = {}): PublicAppInfo => ({
id: 'some-id',
Expand All @@ -52,7 +53,7 @@ describe('toNavLink', () => {
title: 'title',
order: 12,
tooltip: 'tooltip',
euiIconType: 'my-icon',
euiIconType: 'my-icon' as EuiIconType,
}),
basePath
);
Expand All @@ -62,7 +63,7 @@ describe('toNavLink', () => {
title: 'title',
order: 12,
tooltip: 'tooltip',
euiIconType: 'my-icon',
euiIconType: 'my-icon' as EuiIconType,
})
);
});
Expand Down
4 changes: 3 additions & 1 deletion src/core/types/app_category.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
* GitHub history for details.
*/

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';

/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
Expand Down Expand Up @@ -64,5 +66,5 @@ export interface AppCategory {
* If the category is only 1 item, and no icon is defined, will default to the product icon
* Defaults to initials if no icon is defined
*/
euiIconType?: string;
euiIconType?: EuiIconType;
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { i18n } from '@osd/i18n';
import { I18nProvider } from '@osd/i18n/react';
import { StartServicesAccessor } from 'src/core/public';

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { AdvancedSettings } from './advanced_settings';
import { ManagementAppMountParams } from '../../../management/public';
import { ComponentRegistry } from '../types';
Expand All @@ -54,7 +55,7 @@ const readOnlyBadge = {
tooltip: i18n.translate('advancedSettings.badge.readOnly.tooltip', {
defaultMessage: 'Unable to save advanced settings',
}),
iconType: 'glasses',
iconType: 'glasses' as EuiIconType,
};

export async function mountManagementSection(
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/apm_oss/server/tutorial/envs/on_prem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
*/

import { i18n } from '@osd/i18n';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { INSTRUCTION_VARIANT } from '../../../../../../src/plugins/home/server';
import {
createWindowsServerInstructions,
Expand Down Expand Up @@ -82,7 +83,7 @@ export function onPremInstructions({
defaultMessage: `Please make sure your APM Server is updated to 7.0 or higher. \
You can also migrate your 6.x data with the migration assistant found in OpenSearch Dashboards's management section.`,
}),
iconType: 'alert',
iconType: 'alert' as EuiIconType,
},
instructionVariants: [
{
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/apm_oss/server/tutorial/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
*/

import { i18n } from '@osd/i18n';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { onPremInstructions } from './envs/on_prem';
import apmIndexPattern from './index_pattern.json';
import { ArtifactsSchema, TutorialsCategory } from '../../../../../src/plugins/home/server';
Expand Down Expand Up @@ -94,7 +95,7 @@ It allows you to monitor the performance of thousands of applications in real ti
'{config.docs.base_url}guide/en/apm/get-started/{config.docs.version}/index.html',
},
}),
euiIconType: 'apmApp',
euiIconType: 'apmApp' as EuiIconType,
artifacts,
onPrem: onPremInstructions(indices),
previewImagePath: '/plugins/apmOss/assets/apm.png',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { i18n } from '@osd/i18n';
import _ from 'lodash';
import uuid from 'uuid';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ActionByType, IncompatibleActionError } from '../../ui_actions_plugin';
import { ViewMode, PanelState, IEmbeddable } from '../../embeddable_plugin';
import {
Expand Down Expand Up @@ -63,7 +64,7 @@ export class AddToLibraryAction implements ActionByType<typeof ACTION_ADD_TO_LIB
});
}

public getIconType({ embeddable }: AddToLibraryActionContext) {
public getIconType({ embeddable }: AddToLibraryActionContext): EuiIconType {
if (!embeddable.getRoot() || !embeddable.getRoot().isContainer) {
throw new IncompatibleActionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { i18n } from '@osd/i18n';
import { CoreStart } from 'src/core/public';
import uuid from 'uuid';
import _ from 'lodash';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ActionByType, IncompatibleActionError } from '../../ui_actions_plugin';
import { ViewMode, PanelState, IEmbeddable } from '../../embeddable_plugin';
import { SavedObject } from '../../../../saved_objects/public';
Expand Down Expand Up @@ -69,7 +70,7 @@ export class ClonePanelAction implements ActionByType<typeof ACTION_CLONE_PANEL>
});
}

public getIconType({ embeddable }: ClonePanelActionContext) {
public getIconType({ embeddable }: ClonePanelActionContext): EuiIconType {
if (!embeddable.getRoot() || !embeddable.getRoot().isContainer) {
throw new IncompatibleActionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* under the License.
*/

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { i18n } from '@osd/i18n';
import { IEmbeddable } from '../../embeddable_plugin';
import { ActionByType, IncompatibleActionError } from '../../ui_actions_plugin';
Expand Down Expand Up @@ -72,7 +73,7 @@ export class ExpandPanelAction implements ActionByType<typeof ACTION_EXPAND_PANE
});
}

public getIconType({ embeddable }: ExpandPanelActionContext) {
public getIconType({ embeddable }: ExpandPanelActionContext): EuiIconType {
if (!embeddable.parent || !isDashboard(embeddable.parent)) {
throw new IncompatibleActionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import React from 'react';
import { i18n } from '@osd/i18n';
import { EuiBadge } from '@elastic/eui';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import {
IEmbeddable,
ViewMode,
Expand All @@ -55,7 +56,7 @@ export class LibraryNotificationAction implements ActionByType<typeof ACTION_LIB
defaultMessage: 'Library',
});

private icon = 'folderCheck';
private icon = 'folderCheck' as EuiIconType;

public readonly MenuItem = reactToUiComponent(() => (
<EuiBadge
Expand All @@ -76,7 +77,7 @@ export class LibraryNotificationAction implements ActionByType<typeof ACTION_LIB
return this.displayName;
}

public getIconType({ embeddable }: LibraryNotificationActionContext) {
public getIconType({ embeddable }: LibraryNotificationActionContext): EuiIconType {
if (!embeddable.getRoot() || !embeddable.getRoot().isContainer) {
throw new IncompatibleActionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import { i18n } from '@osd/i18n';
import { CoreStart } from 'src/core/public';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { IEmbeddable, ViewMode, EmbeddableStart } from '../../embeddable_plugin';
import { DASHBOARD_CONTAINER_TYPE, DashboardContainer } from '../embeddable';
import { ActionByType, IncompatibleActionError } from '../../ui_actions_plugin';
Expand Down Expand Up @@ -66,11 +67,11 @@ export class ReplacePanelAction implements ActionByType<typeof ACTION_REPLACE_PA
});
}

public getIconType({ embeddable }: ReplacePanelActionContext) {
public getIconType({ embeddable }: ReplacePanelActionContext): EuiIconType {
if (!embeddable.parent || !isDashboard(embeddable.parent)) {
throw new IncompatibleActionError();
}
return 'dqlOperand';
return 'kqlOperand';
}

public async isCompatible({ embeddable }: ReplacePanelActionContext) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { i18n } from '@osd/i18n';
import _ from 'lodash';
import uuid from 'uuid';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ActionByType, IncompatibleActionError } from '../../ui_actions_plugin';
import { ViewMode, PanelState, IEmbeddable } from '../../embeddable_plugin';
import {
Expand Down Expand Up @@ -63,7 +64,7 @@ export class UnlinkFromLibraryAction implements ActionByType<typeof ACTION_UNLIN
});
}

public getIconType({ embeddable }: UnlinkFromLibraryActionContext) {
public getIconType({ embeddable }: UnlinkFromLibraryActionContext): EuiIconType {
if (!embeddable.getRoot() || !embeddable.getRoot().isContainer) {
throw new IncompatibleActionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import { i18n } from '@osd/i18n';
import { AppMountParameters } from 'opensearch-dashboards/public';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ViewMode } from '../../embeddable_plugin';
import { TopNavIds } from './top_nav_ids';
import { NavAction } from '../../types';
Expand Down Expand Up @@ -94,7 +95,7 @@ function getEditConfig(action: NavAction) {
return {
emphasize: true,
id: 'edit',
iconType: 'pencil',
iconType: 'pencil' as EuiIconType,
label: i18n.translate('dashboard.topNave.editButtonAriaLabel', {
defaultMessage: 'edit',
}),
Expand Down Expand Up @@ -183,7 +184,7 @@ function getAddConfig(action: NavAction) {
function getCreateNewConfig(action: NavAction) {
return {
emphasize: true,
iconType: 'plusInCircleFilled',
iconType: 'plusInCircleFilled' as EuiIconType,
id: 'addNew',
label: i18n.translate('dashboard.topNave.addNewButtonAriaLabel', {
defaultMessage: 'Create new',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { i18n } from '@osd/i18n';
import { ApplicationStart } from 'opensearch-dashboards/public';
import { Action } from 'src/plugins/ui_actions/public';
import { take } from 'rxjs/operators';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ViewMode } from '../types';
import { EmbeddableFactoryNotFoundError } from '../errors';
import { EmbeddableStart } from '../../plugin';
Expand Down Expand Up @@ -87,7 +88,7 @@ export class EditPanelAction implements Action<ActionContext> {
});
}

getIconType() {
getIconType(): EuiIconType {
return 'pencil';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { i18n } from '@osd/i18n';
import { Action, ActionExecutionContext } from 'src/plugins/ui_actions/public';
import { NotificationsStart, OverlayStart } from 'src/core/public';
import { EmbeddableStart } from 'src/plugins/embeddable/public/plugin';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ViewMode } from '../../../../types';
import { openAddPanelFlyout } from './open_add_panel_flyout';
import { IContainer } from '../../../../containers';
Expand Down Expand Up @@ -60,7 +61,7 @@ export class AddPanelAction implements Action<ActionContext> {
});
}

public getIconType() {
public getIconType(): EuiIconType {
return 'plusInCircleFilled';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import { i18n } from '@osd/i18n';
import { Action } from 'src/plugins/ui_actions/public';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { ViewMode } from '../../../../types';
import { IEmbeddable } from '../../../../embeddables';

Expand All @@ -56,7 +57,7 @@ export class CustomizePanelTitleAction implements Action<ActionContext> {
});
}

public getIconType() {
public getIconType(): EuiIconType {
return 'pencil';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { i18n } from '@osd/i18n';
import { Action } from 'src/plugins/ui_actions/public';
import { Start as InspectorStartContract } from 'src/plugins/inspector/public';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { IEmbeddable } from '../../../embeddables';

export const ACTION_INSPECT_PANEL = 'openInspector';
Expand All @@ -52,7 +53,7 @@ export class InspectPanelAction implements Action<ActionContext> {
});
}

public getIconType() {
public getIconType(): EuiIconType {
return 'inspect';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
*/

import { i18n } from '@osd/i18n';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { Action, IncompatibleActionError } from '../../../ui_actions';
import { ContainerInput, IContainer } from '../../../containers';
import { ViewMode } from '../../../types';
Expand Down Expand Up @@ -63,7 +64,7 @@ export class RemovePanelAction implements Action<ActionContext> {
});
}

public getIconType() {
public getIconType(): EuiIconType {
return 'trash';
}

Expand Down
Loading