-
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 fields editor] Expose editor for consuming apps #82116
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this component only renders the <RuntimeFieldForm />
but in a later stage it will also render the "Preview field" functionality.
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 ! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryankeairns Is this what you're thinking? elastic/eui#499
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some subjectivity, but yeah... in this case rtfEditor
is the entity/block and the form is the element.
@@ -119,7 +123,7 @@ const RuntimeFieldFormComp = ({ defaultValue, onChange, docsBaseUri }: Props) => | |||
fullWidth | |||
> | |||
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/** Width of editor. Defaults to 100%. */ | |
width?: string | number; |
💚 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: |
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.
A bump to 40k is totally fine, we plan to limit all bundles to 200k in the future.
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 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.tsx
addx-pack/plugins/index_management/public/application/mount_management_section.ts
addoverlays