-
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
[Mappings editor] Support for dynamic templates #54523
[Mappings editor] Support for dynamic templates #54523
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
…nto mappings-editor/dynamic-templates
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.
Awesome work @alisonelizabeth ! I love to see how easy it is now to add those initially complex data flow of validating + merging data to build our final object. 👍
I just made 2 comments that should be easy to address. Cheers!
...anagement/public/app/components/mappings_editor/components/templates_form/templates_form.tsx
Show resolved
Hide resolved
defaultValue: { | ||
dynamic_templates: MappingsTemplates['dynamic_templates']; | ||
}; | ||
form?: FormHook<MappingsTemplates>; |
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 made a change on my current PR in the logic of the configuration form to be more specific instead of passing the whole form
. You can see it here: https://github.com/elastic/kibana/pull/54241/files#diff-c7f4f4960afc300ef126e2c1a3d857b8R48
Can we make the same change here?
It also means to be more specific when we dispatch the update, as you can see here: https://github.com/elastic/kibana/pull/54241/files#diff-dfcd0122417fdb550ad2c83268216bd6R96
As a bonus, @cjcenizal will love you for that! 😊
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 happy to make this change, however, I see it will also require some of the changes you made to mappings_editor.tsx
. I think it might make more sense to wait to adopt this until after your PR is merged.
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.
Tested locally and the UX looks good! Thanks for the fast turnaround, @alisonelizabeth. I did notice that it looks like the help text example is a little misleading. Maybe we could just remove the help text entirely, since people who are here will either visit the docs and see the examples or they'll already be familiar with the expected format?
component={JsonEditorField} | ||
componentProps={{ | ||
euiCodeEditorProps: { | ||
height: '400px', |
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.
This is a pretty narrow window, and this object can get fairly long. How would you feel about bumping this up just a bit, e.g. to 600px?
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This PR adds support for dynamic templates within the mappings editor.
Screenshots
How to test