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

[ResponseOps] Migrate EUI CodeEditor in the triggers actions ui to use the monaco based editor #122734

Merged
merged 3 commits into from
Jan 26, 2022
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { useEffect } from 'react';

export const mockEditorInstance = {
executeEdits: () => {},
getSelection: () => {},
getValue: () => {},
onDidBlurEditorWidget: () => ({
dispose: () => {},
}),
};

export const MockCodeEditor = (props: any) => {
const { editorDidMount } = props;
useEffect(() => {
editorDidMount(mockEditorInstance);
}, [editorDidMount]);

return (
<input
data-test-subj={props['data-test-subj'] || 'mockCodeEditor'}
data-value={props.value}
value={props.value}
onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
props.onChange(e.target.value);
}}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,22 @@ import { mountWithIntl, nextTick } from '@kbn/test/jest';
import { act } from '@testing-library/react';
import ParamsFields from './es_index_params';
import { AlertHistoryEsIndexConnectorId } from '../../../../types';
import { MockCodeEditor } from '../../../code_editor.mock';

const kibanaReactPath = '../../../../../../../../src/plugins/kibana_react/public';

jest.mock('../../../../common/lib/kibana');

jest.mock(kibanaReactPath, () => {
const original = jest.requireActual(kibanaReactPath);
return {
...original,
CodeEditor: (props: any) => {
return <MockCodeEditor {...props} />;
},
};
});

const actionConnector = {
actionTypeId: '.index',
config: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,19 @@
import React from 'react';
import { mountWithIntl } from '@kbn/test/jest';
import WebhookParamsFields from './webhook_params';
import { MockCodeEditor } from '../../../code_editor.mock';

const kibanaReactPath = '../../../../../../../../src/plugins/kibana_react/public';

jest.mock(kibanaReactPath, () => {
const original = jest.requireActual(kibanaReactPath);
return {
...original,
CodeEditor: (props: any) => {
return <MockCodeEditor {...props} />;
},
};
});

describe('WebhookParamsFields renders', () => {
test('all params fields is rendered', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,22 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { mountWithIntl } from '@kbn/test/jest';
import { JsonEditorWithMessageVariables } from './json_editor_with_message_variables';
import { MockCodeEditor } from '../code_editor.mock';

const kibanaReactPath = '../../../../../../src/plugins/kibana_react/public';

jest.mock(kibanaReactPath, () => {
const original = jest.requireActual(kibanaReactPath);
return {
...original,
CodeEditor: (props: any) => {
return <MockCodeEditor {...props} />;
},
};
});

describe('JsonEditorWithMessageVariables', () => {
const onDocumentsChange = jest.fn();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,34 @@
* 2.0.
*/

import React, { useState } from 'react';
import { EuiFormRow } from '@elastic/eui';
import React, { useCallback, useEffect, useRef, useState } from 'react';
import { EuiFormRow, EuiCallOut, EuiSpacer } from '@elastic/eui';

import { XJsonMode } from '@kbn/ace';
import 'brace/theme/github';
import { i18n } from '@kbn/i18n';
import { monaco, XJsonLang } from '@kbn/monaco';

import './add_message_variables.scss';
import { XJson, EuiCodeEditor } from '../../../../../../src/plugins/es_ui_shared/public';
import { XJson } from '../../../../../../src/plugins/es_ui_shared/public';
import { CodeEditor } from '../../../../../../src/plugins/kibana_react/public';

import { AddMessageVariables } from './add_message_variables';
import { ActionVariable } from '../../../../alerting/common';
import { templateActionVariable } from '../lib';

const NO_EDITOR_ERROR_TITLE = i18n.translate(
'xpack.triggersActionsUI.components.jsonEditorWithMessageVariable.noEditorErrorTitle',
{
defaultMessage: 'Unable to add message variable',
}
);

const NO_EDITOR_ERROR_MESSAGE = i18n.translate(
'xpack.triggersActionsUI.components.jsonEditorWithMessageVariable.noEditorErrorMessage',
{
defaultMessage: 'Editor was not found, please refresh page and try again',
}
);

interface Props {
messageVariables?: ActionVariable[];
paramsProperty: string;
Expand All @@ -31,7 +46,11 @@ interface Props {
}

const { useXJsonMode } = XJson;
const xJsonMode = new XJsonMode();

// Source ID used to insert text imperatively into the code editor,
// this value is only used to uniquely identify any single edit attempt.
// Multiple editors can use the same ID without any issues.
const EDITOR_SOURCE = 'json-editor-with-message-variables';
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is probably down to me not being familiar with monaco, but what is the impact of this source string?
Does it denote some kind of unique identifier?
What happens if there are multiple monaco editors on the page and they all use this same string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just something to uniquely ID the edit I believe. Multiple editors do no share the same editor reference, so you can use the same source ID

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha, I think an improved comment to this effect would help here

Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple editors do no share the same editor reference, so you can use the same source ID

Cool :)
Lets verify ;) but sound good 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried several on the same page and it looks great :)


export const JsonEditorWithMessageVariables: React.FunctionComponent<Props> = ({
messageVariables,
Expand All @@ -44,29 +63,83 @@ export const JsonEditorWithMessageVariables: React.FunctionComponent<Props> = ({
helpText,
onBlur,
}) => {
const [cursorPosition, setCursorPosition] = useState<any>(null);
const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>();
const editorDisposables = useRef<monaco.IDisposable[]>([]);
const [showErrorMessage, setShowErrorMessage] = useState(false);

const { convertToJson, setXJson, xJson } = useXJsonMode(inputTargetValue ?? null);

const onSelectMessageVariable = (variable: ActionVariable) => {
const editor = editorRef.current;
if (!editor) {
setShowErrorMessage(true);
return;
}
const cursorPosition = editor.getSelection();
const templatedVar = templateActionVariable(variable);

let newValue = '';
if (cursorPosition) {
const cursor = cursorPosition.getCursor();
cursorPosition.session.insert(cursor, templatedVar);
newValue = cursorPosition.session.getValue();
editor.executeEdits(EDITOR_SOURCE, [
{
range: cursorPosition,
text: templatedVar,
},
]);
newValue = editor.getValue();
} else {
newValue = templatedVar;
}
setShowErrorMessage(false);
setXJson(newValue);
// Keep the documents in sync with the editor content
onDocumentsChange(convertToJson(newValue));
};

const onClickWithMessageVariable = (_value: any) => {
setCursorPosition(_value);
const registerEditorListeners = useCallback(() => {
const editor = editorRef.current;
if (!editor) {
return;
}
Comment on lines +100 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets think about what happens if the editor is null here...
We don't want nothing to happen, as that leaves users confused and unable to progress.
An explicit error is preferred as they can report it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea sure I'm open to suggestions, although this is a guard for the initial register call since we're reusing this method in the useEffect, which will call this method once before we get a reference to the editor.

We could add a console.error/warn, or maybe split it into 2 cases, 1 where the editor exists, another when it doesn't exist, just to be more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I wonder if we can show something in the UI in this scenario - some kind of <EuiCallout> to indicate what the problem and what the user can do about it (I'm not sure if we even know this TBH)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd opt for a UX that the user sees so that they know something went wrong and they can report it.

editorDisposables.current.push(
editor.onDidBlurEditorWidget(() => {
onBlur?.();
})
);
}, [onBlur]);

const unregisterEditorListeners = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there example code somewhere for how to use this new editor? I tried following the issue trail but didn't see much in terms of "this is how you implement it"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is some precedence for this pattern in the lens plugin formula_editor.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I don't really know how to review this part honestly. It seems okay but is it worth reaching out to folks who know more about the EUI CodeEditor to make sure this is looks right (no edge cases unresolved, no memory leaks, etc)?

editorDisposables.current.forEach((d) => {
d.dispose();
});
editorDisposables.current = [];
};

const onEditorMount = (editor: monaco.editor.IStandaloneCodeEditor) => {
editorRef.current = editor;
registerEditorListeners();
};

const renderErrorMessage = () => {
if (!showErrorMessage) {
return null;
}
return (
<>
<EuiSpacer size="s" />
<EuiCallOut size="s" color="danger" iconType="alert" title={NO_EDITOR_ERROR_TITLE}>
<p>{NO_EDITOR_ERROR_MESSAGE}</p>
</EuiCallOut>
<EuiSpacer size="s" />
</>
);
};

useEffect(() => {
registerEditorListeners();
return () => unregisterEditorListeners();
}, [registerEditorListeners]);

return (
<EuiFormRow
fullWidth
Expand All @@ -82,22 +155,36 @@ export const JsonEditorWithMessageVariables: React.FunctionComponent<Props> = ({
}
helpText={helpText}
>
<EuiCodeEditor
mode={xJsonMode}
width="100%"
height="200px"
theme="github"
data-test-subj={`${paramsProperty}JsonEditor`}
aria-label={areaLabel}
value={xJson}
onChange={(xjson: string, e: any) => {
setXJson(xjson);
// Keep the documents in sync with the editor content
onDocumentsChange(convertToJson(xjson));
}}
onCursorChange={(_value: any) => onClickWithMessageVariable(_value)}
onBlur={onBlur}
/>
<>
{renderErrorMessage()}
<CodeEditor
languageId={XJsonLang.ID}
options={{
renderValidationDecorations: xJson ? 'on' : 'off', // Disable error underline when empty
lineNumbers: 'on',
fontSize: 14,
minimap: {
enabled: false,
},
scrollBeyondLastLine: false,
folding: true,
wordWrap: 'on',
wrappingIndent: 'indent',
automaticLayout: true,
}}
value={xJson}
width="100%"
height="200px"
data-test-subj={`${paramsProperty}JsonEditor`}
aria-label={areaLabel}
editorDidMount={onEditorMount}
onChange={(xjson: string) => {
setXJson(xjson);
// Keep the documents in sync with the editor content
onDocumentsChange(convertToJson(xjson));
}}
/>
</>
</EuiFormRow>
);
};