-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add EuiCodeEditor to ES UI Shared. #108318
Conversation
307f2c2
to
148ac47
Compare
…r. Remove warning from EuiCodeEditor.
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
@@ -336,7 +336,7 @@ | |||
"re-resizable": "^6.1.1", | |||
"re2": "^1.15.4", | |||
"react": "^16.12.0", | |||
"react-ace": "^5.9.0", | |||
"react-ace": "^7.0.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timroes Any suggestions on the best way to verify this hasn't broken anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have any better way besides manually testing a couple of places we use the editor. I'd suggest looking into at least one read only case, one write case, and advanced settings (which I think is the only place using dynamic heights on EuiCodeEditor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Tim! Tested Advanced Settings to cover both the write and dynamic heights use cases, and tested the Rollup Jobs JSON tab for the read-only case. All seems to behave as expected.
|
||
// TODO: Fix this. document.activeElement resolves to be <body />, but the component behaves | ||
// as expected in the browser. | ||
test.skip('pressing escape in ace textbox will enable overlay', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chandlerprall Do you or anyone on the EUI team have any suggestions as to why this test might be failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kibana's version of jsdom (16.4.0) is newer than EUI's version (11.12.0), and the newer one is preventing the call to .focus()
from doing work because of this additional check: https://github.com/jsdom/jsdom/blob/16.4.0/lib/jsdom/living/helpers/focusing.js#L25-L27
if (!elImpl.isConnected) {
return false;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This helped a ton.
@elasticmachine merge upstream |
a99f801
to
b012460
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cjcenizal,
thank you so much for creating this workaround!
I tested locally the 3 apps: Ingest pipelines, Index Management and Grok debugger and haven't noticed any regressions. Do we need to migrate the rest of the apps listed in #107215?
For the mocks in the test files, I think maybe a global mock file in es_ui_shared
that can be imported everywhere could work better. Similar to a mock file in ILM that is being imported when needed. But I don't want to block on it.
Thanks for your review @yuliacech! I will migrate the rest of our apps in a separate PR. I like your suggestion of a global mock file and I think it's worth implementing. The challenge is that some of the mocks are subtly different than others, but I'll see if I can reconcile their differences. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
async chunk count
History
To update your PR or re-run it, just comment with: |
* Export EuiCodeEditor from es_ui_shared and consume it in Grok Debugger. Remove warning from EuiCodeEditor. * Lazy-load code editor so it doesn't bloat the EsUiShared plugin bundle. * Refactor mocks into a shared jest_mock.tsx file.
This PR transplants the
EuiCodeEditor
component from EUI to thees_ui_shared
plugin. This is a stopgap solution for removal of this component from EUI before we get to #107215.Developers can consume this component by importing it:
I suggest creating a file called
shared_imports.ts
in the root of your plugin'spublic
directory and re-exporting it there, for consumption by your plugin.This PR also:
src/plugins/es_ui_shared/public/components/json_editor/json_editor.tsx
to consume this transplantedEuiCodeEditor
.JsonEditor
is consumed by Index Management and Ingest Node Pipelines.Lazy-loading
This component is lazy-loaded to prevent it from bloating the EsUiShared plugin bundle. Here's how it appears when the network has been throttled to simulate a "Slow 3G" connection.
Steps to test
This is a drop-in replacement for EuiCodeEditor since it's identical code. Testers can interact with Grok Debugger and the JSON editor in the Ingest Node Pipelines "Load from JSON" modal to verify that this change hasn't broken anything.