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

JiaweiWu · 2022-01-12T01:19:28Z

Resolves #107203

The JSON code editor in the triggers actions UI now uses the monaco based editor.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

elasticmachine · 2022-01-12T01:19:30Z

Pinging @elastic/response-ops (Team:ResponseOps)

gmmorris · 2022-01-12T10:25:32Z

...ins/triggers_actions_ui/public/application/components/json_editor_with_message_variables.tsx

+    const editor = editorRef.current;
+    if (!editor) {
+      return;
+    }


What would the UX be if the monaco reference is lost for some reason?
Will it just render an empty space?

Yea good question, so this is mainly here to appease typescript's null checks (instead of doing reference checks all the time).

In theory this would never be called if editor reference is unavailable since we maintain the reference as soon as the component mounts. There isn't any way to lose the reference right now.

But yea if somehow we lost a reference, this function would no-op and the contents within the editor would remain unchanged

Fair enough :)
In that case, I think we should throw, so that if an edgecase happens we'll at least show the user some feedback :)

gmmorris · 2022-01-12T11:56:18Z

...ins/triggers_actions_ui/public/application/components/json_editor_with_message_variables.tsx

@@ -31,7 +31,8 @@ interface Props {
 }

 const { useXJsonMode } = XJson;
-const xJsonMode = new XJsonMode();
+// Source used to insert text imperatively into the code editor
+const EDITOR_SOURCE = 'json-editor-with-message-variables';


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?

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

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

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

Cool :)
Lets verify ;) but sound good 👍

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

gmmorris · 2022-01-12T12:29:20Z

...ins/triggers_actions_ui/public/application/components/json_editor_with_message_variables.tsx

+    const editor = editorRef.current;
+    if (!editor) {
+      return;
+    }


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.

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

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)

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

JiaweiWu · 2022-01-17T22:24:46Z

Screenshot of the new error message

chrisronline · 2022-01-20T19:26:47Z

...ins/triggers_actions_ui/public/application/components/json_editor_with_message_variables.tsx

+    );
+  }, [onBlur]);
+
+  const unregisterEditorListeners = () => {


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"

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

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)?

…aco based editors

kibana-ci · 2022-01-25T08:24:08Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	333	320	-13

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	782.8KB	674.8KB	-107.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	53.9KB	53.9KB	+7.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	67	66	-1

History

💔 Build #17881 failed acc17fbd56f32bada508dcfce268a1f6b198f80d
💔 Build #17141 failed 0a516ee826b2d7cbfa4616df3e986c3ec2bd7082
💔 Build #16870 failed 25e0e9057e143cc5d9a31d3db5f99fa07259f07d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

crob611

Looked over the Code Editor usage and all looked good to me 👍

JiaweiWu requested a review from gmmorris January 12, 2022 01:19

JiaweiWu added the release_note:skip Skip the PR/issue when compiling release notes label Jan 12, 2022

gmmorris reviewed Jan 12, 2022

View reviewed changes

chrisronline reviewed Jan 20, 2022

View reviewed changes

chrisronline requested a review from crob611 January 24, 2022 19:22

JiaweiWu added 3 commits January 24, 2022 22:26

Migrate the EUI code editor in the triggers actions ui to use the mon…

2fead44

…aco based editors

Refactor mock code editor, fixed unit tests

5bb9fd3

Addressed comments

62de0fe

JiaweiWu added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Jan 26, 2022

crob611 approved these changes Jan 26, 2022

View reviewed changes

chrisronline approved these changes Jan 26, 2022

View reviewed changes

JiaweiWu merged commit 4d3a792 into elastic:main Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JiaweiWu commented Jan 12, 2022

elasticmachine commented Jan 12, 2022

gmmorris Jan 12, 2022

JiaweiWu Jan 12, 2022

gmmorris Jan 13, 2022

gmmorris Jan 12, 2022

JiaweiWu Jan 12, 2022

chrisronline Jan 13, 2022

gmmorris Jan 13, 2022

gmmorris Jan 14, 2022

gmmorris Jan 12, 2022

JiaweiWu Jan 12, 2022

chrisronline Jan 13, 2022

gmmorris Jan 13, 2022

JiaweiWu commented Jan 17, 2022

chrisronline Jan 20, 2022

JiaweiWu Jan 24, 2022

chrisronline Jan 24, 2022

kibana-ci commented Jan 25, 2022

async chunk count

crob611 left a comment

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

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

Conversation

JiaweiWu commented Jan 12, 2022

Checklist

elasticmachine commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaweiWu commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jan 25, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

crob611 left a comment

Choose a reason for hiding this comment