[Runtime fields editor] Expose editor for consuming apps#82116
Conversation
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
| export const RuntimeFieldEditor = ({ defaultValue, onChange, docLinks }: Props) => { | ||
| const links = getLinks(docLinks); | ||
|
|
||
| return <RuntimeFieldForm links={links} defaultValue={defaultValue} onChange={onChange} />; |
There was a problem hiding this comment.
For now, this component only renders the <RuntimeFieldForm /> but in a later stage it will also render the "Preview field" functionality.
jloleysens
left a comment
There was a problem hiding this comment.
Great work @sebelga ! Code looks good to me! Also tested locally. TIL about core.overalys.openFlyout.
As an aside, what are the original design decisions behind a service like this? It seems to be a way for UI rendering code to open a flyout.
| })(props) as TestBed; | ||
|
|
||
| const docLinks: DocLinksStart = { | ||
| ELASTIC_WEBSITE_URL: 'htts://jestTest.elastic.co', |
There was a problem hiding this comment.
I know this is just a test value, but looks like there is a typo in htts
| <EuiButton onClick={() => setIsFlyoutVisible(true)}>Create field</EuiButton> | ||
|
|
||
| {isFlyoutVisible && ( | ||
| <EuiFlyout onClose={() => setIsFlyoutVisible(false)}> |
There was a problem hiding this comment.
I think I recall us talking about something similar to this in the past but why is the RuntimeFieldEditorFlyout wrapped in EuiFlyout?
There was a problem hiding this comment.
So it can be injected in a flyout when calling core.overalys.openFlyout(). It is a similar mechanism that I put in place for the <GlobalFlyout />.
There was a problem hiding this comment.
Maybe renaming RuntimeFieldEditorFlyout to RuntimeFieldEditorFlyoutContent would make the relationship clearer? Otherwise it is a bit of a head-scratcher since the code makes it look like a flyout is being putting inside another flyout.
There was a problem hiding this comment.
Good idea, I will rename the component 👍
|
@sebelga out of curiosity, were you working with a designer on this? I'm asking to better understand the depth of review needed here from the design team. Thanks! |
|
Thanks for the review @jloleysens !
I think we added it when "actions and triggers" got released. With those, we can register dynamic action to panels, so we needed a way to programmatically open a flyout. @ryankeairns Yes, we worked with @andreadelrio on this. The ping for the design team is because I added a .scss file. So I guess that's the only thing you'd need to review 😊. Of course, any other feedback is welcome! |
|
@mistic What should I do about those |
ryankeairns
left a comment
There was a problem hiding this comment.
Added a couple of quick observations regarding class naming and file organization. That said, I'm curious about the backgroud color override and will fire this locally to take a peek.
|
|
||
| import { schema } from './schema'; | ||
|
|
||
| import './runtime_field_form.scss'; |
There was a problem hiding this comment.
Instead of importing component-level SCSS files directly here, please create an index.scss and import it in your main application file. Commonly, the index.scss lives at public/ but it can also be in your components folder, if necessary. Main point being, we want to avoid the importation of multiple component level files in multiple places.
There was a problem hiding this comment.
On second thought...
I'm thinking we might be able to avoid any SCSS overrides altogether in this PR. In looking at this locally, it strikes me that - since this is akin to a form input - we should be coloring it as such by default.
Specifically, I'm inclined to change the default editor theme's background color to $euiFormBackgroundColor in src/plugins/kibana_react/public/code_editor/editor_theme.ts (from its current empty/white color). This means we'd have to look at other instances of this CodeEditor component, but I suspect it's only used in a few spots (and it may not result in any changes for those instances any way).
If there are no objections, then you can remove your scss file, leave it with a white background for now, and I'll start a separate PR that changes the theme.
cc:/ @poffdeluxe since he originally built this for Canvas (I can clean up the container bg color there)
There was a problem hiding this comment.
@ryankeairns Is this what you're thinking? elastic/eui#499
There was a problem hiding this comment.
Instead of importing component-level SCSS files directly here, please create an index.scss and import it in your main application file.
I am a bit surprised here as it feels like going backward. We used to do that, then the recommended way (I think @cchaos mentioned it) was to keep the .scss file close to the component and import it directly from the component file. As we do here
- https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/processor_output/processor_output.tsx#L23
- https://github.com/elastic/kibana/blob/master/x-pack/plugins/index_management/public/application/components/component_templates/component_template_selector/component_templates_selector.tsx#L25)
I like that approach as it makes clearer the connection of the component with its style and we'd probably get the CSS lazyloaded if the component is lazyloaded. So now we need to go back and declare a top-level index.scss?
I'm thinking we might be able to avoid any SCSS overrides altogether in this PR. In looking at this locally, it strikes me that - since this is akin to a form input - we should be coloring it as such by default.
That'd be awesome if we could theme this globally. I did not want the runtime field editor to ship with a white background and no border as it lacked a frame for the user to work with. And from @cjcenizal PR I see the issue is from 2018 so I did not want to hold my breath on it 😄
But if you'll work on it I will remove the .scss override on this PR then. 👍
There was a problem hiding this comment.
Yes, the current spec for adding new SASS files is to import them directly into the matching JS component file. https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#sass-files
| @@ -0,0 +1,5 @@ | |||
| .runtimeFieldEditor_form { | |||
There was a problem hiding this comment.
Our css class naming follows the BEM schema and is made unique with a three-letter prefix to avoid style conflicts across applications. In this case, I would propose using something like .rtfEditor
There was a problem hiding this comment.
Cool. Just so I understand you correctly, if I was going to correct this it would be .rtfEditor__form and not .rtfEditor right? As "form" is a child of the editor.
There was a problem hiding this comment.
There's some subjectivity, but yeah... in this case rtfEditor is the entity/block and the form is the element.
| <CodeEditor | ||
| languageId="painless" | ||
| languageId={PainlessLang.ID} | ||
| // 99% width allows the editor to resize horizontally. 100% prevents it from resizing. |
There was a problem hiding this comment.
@sebelga can you explain how to reproduce this?
I've tried this at 100% and 99% but am seeing what seems to be identical behavior (likely missing something). Thanks!
There was a problem hiding this comment.
I think this might be inherited from https://github.com/elastic/kibana/pull/57538/files#diff-74f90896d62130f9108f9f6a68490f6cd9042ab39eaf4911145f5eb3b8de986bR18. It occurs in Painless Lab because the different panes (so tempted to riff on a pun here) resize as you change window width. One of those panes contains a <CodeEditor>.
There was a problem hiding this comment.
Actually, it was taken from the mappings editor "runtime" field type from https://github.com/elastic/kibana/pull/79940/files#diff-86ba24cfc405b9dded48e7e1eb210143eef125c734cbdc130f5bc3ddf95d2d6aR30
I will remove it here and in the mappings editor if there are not needed in those contexts. 👍
|
@elasticmachine merge upstream |
…kibana into runtime-fields/flyout-component
|
Thanks @mistic ! I've updated the plugin size limit. Can you have a look? @ryankeairns I've addressed your comments, can you have another look? Cheers! |
| // 99% width allows the editor to resize horizontally. 100% prevents it from resizing. | ||
| width="99%" | ||
| languageId={PainlessLang.ID} | ||
| width="100%" |
There was a problem hiding this comment.
You can safely remove the width prop and it will default to 100%
kibana/src/plugins/kibana_react/public/code_editor/code_editor.tsx
Lines 31 to 32 in a49473b
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
spalger
left a comment
There was a problem hiding this comment.
A bump to 40k is totally fine, we plan to limit all bundles to 200k in the future.
ryankeairns
left a comment
There was a problem hiding this comment.
Thanks for cleaning up the CSS stuff!
|
Thanks for the review @ryankeairns ! |
This PR adds the
<RuntimeFieldEditor />and<RuntimeFieldEditorFlyout />component with their tests.I've also added a README.md with an explanation on how to integrate the editor in the consuming apps.
Consuming the editor in a flyout
How to test
x-pack/plugins/index_management/public/application/app_context.tsxaddx-pack/plugins/index_management/public/application/mount_management_section.tsaddoverlays