-
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
Changes from all commits
67ce18b
f2779f4
9ef4d07
eea0489
b392722
a1b2459
7808ce3
6531283
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
import React from 'react'; | ||
import { CoreSetup, OverlayRef } from 'src/core/public'; | ||
|
||
import { toMountPoint, createKibanaReactContext } from './shared_imports'; | ||
import { LoadEditorResponse, RuntimeField } from './types'; | ||
|
||
export interface OpenRuntimeFieldEditorProps { | ||
onSave(field: RuntimeField): void; | ||
defaultValue?: RuntimeField; | ||
} | ||
|
||
export const getRuntimeFieldEditorLoader = (coreSetup: CoreSetup) => async (): Promise< | ||
LoadEditorResponse | ||
> => { | ||
const { RuntimeFieldEditorFlyoutContent } = await import('./components'); | ||
const [core] = await coreSetup.getStartServices(); | ||
const { uiSettings, overlays, docLinks } = core; | ||
const { Provider: KibanaReactContextProvider } = createKibanaReactContext({ uiSettings }); | ||
|
||
let overlayRef: OverlayRef | null = null; | ||
|
||
const openEditor = ({ onSave, defaultValue }: OpenRuntimeFieldEditorProps) => { | ||
const closeEditor = () => { | ||
overlayRef?.close(); | ||
overlayRef = null; | ||
}; | ||
|
||
const onSaveField = (field: RuntimeField) => { | ||
closeEditor(); | ||
onSave(field); | ||
}; | ||
|
||
overlayRef = overlays.openFlyout( | ||
toMountPoint( | ||
<KibanaReactContextProvider> | ||
<RuntimeFieldEditorFlyoutContent | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. So here there is no GlobalFlyout as it makes use of |
||
onSave={onSaveField} | ||
onCancel={() => overlayRef?.close()} | ||
docLinks={docLinks} | ||
defaultValue={defaultValue} | ||
/> | ||
</KibanaReactContextProvider> | ||
) | ||
); | ||
|
||
return closeEditor; | ||
}; | ||
|
||
return { | ||
openEditor, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { CoreSetup } from 'src/core/public'; | ||
import { coreMock } from 'src/core/public/mocks'; | ||
|
||
jest.mock('../../../../src/plugins/kibana_react/public', () => { | ||
const original = jest.requireActual('../../../../src/plugins/kibana_react/public'); | ||
|
||
return { | ||
...original, | ||
toMountPoint: (node: React.ReactNode) => node, | ||
}; | ||
}); | ||
|
||
import { StartPlugins, PluginStart } from './types'; | ||
import { RuntimeFieldEditorFlyoutContent } from './components'; | ||
import { RuntimeFieldsPlugin } from './plugin'; | ||
|
||
const noop = () => {}; | ||
|
||
describe('RuntimeFieldsPlugin', () => { | ||
let coreSetup: CoreSetup<StartPlugins, PluginStart>; | ||
let plugin: RuntimeFieldsPlugin; | ||
|
||
beforeEach(() => { | ||
plugin = new RuntimeFieldsPlugin(); | ||
coreSetup = coreMock.createSetup(); | ||
}); | ||
|
||
test('should return a handler to load the runtime field editor', async () => { | ||
const setupApi = await plugin.setup(coreSetup, {}); | ||
expect(setupApi.loadEditor).toBeDefined(); | ||
}); | ||
|
||
test('once it is loaded it should expose a handler to open the editor', async () => { | ||
const setupApi = await plugin.setup(coreSetup, {}); | ||
const response = await setupApi.loadEditor(); | ||
expect(response.openEditor).toBeDefined(); | ||
}); | ||
|
||
test('should call core.overlays.openFlyout when opening the editor', async () => { | ||
const openFlyout = jest.fn(); | ||
const onSaveSpy = jest.fn(); | ||
|
||
const mockCore = { | ||
overlays: { | ||
openFlyout, | ||
}, | ||
uiSettings: {}, | ||
}; | ||
coreSetup.getStartServices = async () => [mockCore] as any; | ||
const setupApi = await plugin.setup(coreSetup, {}); | ||
const { openEditor } = await setupApi.loadEditor(); | ||
|
||
openEditor({ onSave: onSaveSpy }); | ||
|
||
expect(openFlyout).toHaveBeenCalled(); | ||
|
||
const [[arg]] = openFlyout.mock.calls; | ||
expect(arg.props.children.type).toBe(RuntimeFieldEditorFlyoutContent); | ||
|
||
// We force call the "onSave" prop from the <RuntimeFieldEditorFlyoutContent /> component | ||
// and make sure that the the spy is being called. | ||
// Note: we are testing implementation details, if we change or rename the "onSave" prop on | ||
// the component, we will need to update this test accordingly. | ||
expect(arg.props.children.props.onSave).toBeDefined(); | ||
arg.props.children.props.onSave(); | ||
expect(onSaveSpy).toHaveBeenCalled(); | ||
}); | ||
|
||
test('should return a handler to close the flyout', async () => { | ||
const setupApi = await plugin.setup(coreSetup, {}); | ||
const { openEditor } = await setupApi.loadEditor(); | ||
|
||
const closeEditorHandler = openEditor({ onSave: noop }); | ||
expect(typeof closeEditorHandler).toBe('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.
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.