-
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
[Runtime field editor] Expose handler from plugin to open editor #82464
[Runtime field editor] Expose handler from plugin to open editor #82464
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@mistic I've added the ability to lazy load the runtime field editor. I was expecting some sort of "runtimeFields.chunk" but instead, I see a "kibanaReact" chunk (which the runtime field editor depend on). And I don't see any of the runtime field editor code inside of those chunks. Do you know what I am doing wrong? This is where I lazy load the component: And this is the request made when calling that function |
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.
Great work @sebelga ! I tested locally and this worked as described.
I left a couple of questions, but nothing I want to block on.
|
||
* As the content of a `<EuiFlyout />` | ||
The recommended way to integrate the runtime fields editor is by adding a plugin dependency to the `"runtimeFields"` x-pack plugin. This way you will be able to lazy load the editor when it is required and it will not increment the bundle size of your plugin. |
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 wonder if it is worth explaining here why we want to share the editor in this way. From what I can tell we are giving users a way to imperatively load a JSX element and I'm wondering what the benefits are of this approach vs a static component. Is it so that we can provide a way for plugins to lazy load the component? Happy if that is only reason, I just wanted to make sure I understood the intention correctly.
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.
So yes, the main reason would be the ability to lazy load. So if the editor depends on an external lib it should not increase the page load time.
The second reason is that it simplifies the API as it is not required anymore to provide core services to the editor. And any other plugin dependency is handled automatically instead of having to provide them all through props.
overlayRef = overlays.openFlyout( | ||
toMountPoint( | ||
<KibanaReactContextProvider> | ||
<RuntimeFieldEditorFlyoutContent |
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.
It is up to consumers to implement the global flyout logic, if they want that, right? I'm thinking it could be cool to mention something like that in the README if we haven't already.
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.
So here there is no GlobalFlyout as it makes use of core.overlays.openFlyout
. If we think it is good to use GlobalFlyout because it adds additional functionality (like drill down to other content) then we can update the README.md with some examples.
Thanks for the review @jloleysens ! I will revert the consumer changes commits before merging... 👍 |
💛 Build succeeded, but was flaky
Test FailuresChrome UI Functional Tests.test/functional/apps/discover/_doc_table·ts.discover app discover doc table add and remove columns should remove columns from the tableStandard Out
Stack Trace
Metrics [docs]@kbn/optimizer bundle module count
page load bundle size
History
To update your PR or re-run it, just comment with: |
This PR update the
setup()
method from the runtime fields plugin to expose aloadEditor
handler.This method lazy loads the runtime field editor components and return a handler to then open the editor.
The README.md has been updated.
How to integrate for consumers
How to test
Navigate to the index management app and click on the "Create field" button.
Note: I've pushed to the branch a commit (Setup consumer (to be reverted)) with changes on index_management. This commit will be reverted after the review so all changes to "index_management" should not be reviewed.