-
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] Form UI #81766
[Runtime fields editor] Form UI #81766
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
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.
Changes in files under operations team code owners LGTM
Thanks for the review @mistic ! Do you know why the build is failing? Not sure I understand the error:
|
@elasticmachine merge upstream |
@sebelga the problem is that baselines for each commit to the |
Exploring a solution in #81938, mind giving me a few hours to see how feasible this solution is? |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
#81938 changed the CI metrics check to only fail if the PR is merging into a tracked branch, like master. The If a similar error was to occur on a branch targeting master then the report would say it was failed. |
Awesome, thanks for looking into this @spalger ! |
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! It's exciting to start seeing this take shape. I left a few comments, nothing blocking. Testing locally and worked as expected.
@@ -460,6 +460,10 @@ Elastic. | |||
|Welcome to the Kibana rollup plugin! This plugin provides Kibana support for Elasticsearch's rollup feature. Please refer to the Elasticsearch documentation to understand rollup indices and how to create rollup jobs. | |||
|
|||
|
|||
|{kib-repo}blob/{branch}/x-pack/plugins/runtime_fields[runtimeFields] | |||
|WARNING: Missing README. |
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.
Can we add a readme? I understand it might not contain much info at this point and is subject to change.
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 have the README.md ready in my following PR 😊
<EuiFlexGroup justifyContent="flexEnd"> | ||
<EuiFlexItem grow={false}> | ||
<EuiLink | ||
href={`${docsBaseUri}/to-be-defined`} |
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'd prefer to comment the link component out for now, rather than merge it with an invalid href.
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 have updated the href in my following PR to point correctly to https://www.elastic.co/guide/en/elasticsearch/painless/master/painless-lang-spec.html
fullWidth | ||
> | ||
<CodeEditor | ||
languageId="painless" |
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 should be able to import the language id from kbn-monaco now.
import { PainlessLang } from '@kbn-monaco';
<CodeEditor
languageId={PainlessLang.ID}
....
/>
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 to know, I will update it. Although IMO it should be the CodeEditor
that should have the languageId
correctly typed with available options ('painless'|'other'
)
return ( | ||
<> | ||
<EuiFormRow label={label} fullWidth> | ||
<EuiComboBox |
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'm not as familiar with the proposed design - but would the EuiSelect
suffice here, as there are only 6 supported types?
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, I've mirrored what CJ did for the mappings editor "runtime" field type in #79940
I'm happy to revisit this later if we don't think the ComboBox brings value 👍
Thanks for the review @alisonelizabeth ! I've addressed your comments in my following PR |
This PR contains the UI component of the runtime field editor form. It is a presentational component that wraps the form with the 3 required fields:
Interface
The component has the following interface
The component is not meant to be consumed directly, there will be another PR with the "container" component that will act as a layer between the apps consuming the runtime field editor and this UI component.
I've also made the x-pack "runtime_fields" plugin a bundle dependency of the "index_management" plugin and imported from there the constants and TS type required for the mappings editor "runtime" field type.
Note: The flyout, EuiCodeBlock and "Update" button are not part of what is about to be merged. This PR is only the UI form component for the runtime field.
How to test
Replace the content of the
x-pack/plugins/index_management/public/application/sections/home/home.tsx
file with the code below and navigate to the "index management" app.