-
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
[index patterns] Index pattern creation modal API #99876
Comments
Pinging @elastic/kibana-app-services (Team:AppServices) |
In particular, I'd like feedback from @weltenwort @flash1293 @timroes |
I believe we are doing flyouts not in the best way in many places in Kibana. In particular, it would be great to open flyouts in declerative way, same as it is provided by EUI library. For example: const MyComponent = () => {
const { indexPatternEditor } = useAppPlugins();
const [open, setOpen] = React.useState(false);
return (
<>
<button onClick={() => setOpen(true)}>Add index pattern</button>
{open && <indexPatternEditor.Flyout onSave={() => {}} />}
</>
)
} It would be great to implement the |
I think an imperative API should be provided because it's also usable outside of React which can sometimes become necessary (e.g. inside of an action handler). While it's possible to use the react api in this situation as well, it's cumbersome and IMHO we shouldn't push this to the consumer. If we feel strongly about the "manual" flyout reference management (I agree it can lead to unclosed flyouts leaking out of the app which is also not great), I would vote for providing two APIs (imperative and declarative). This is also consistent what we are doing for the field deletion modal:
Besides that I'm pretty happy with this API. Didn't discuss this with design or anything, but we could add a pre-set for the index pattern name to the context: openEditor({
onSave,
initalPatternString,
}); This could be used for a user flow like this:
|
If you go for imperative approach maybe worth creating a helper, say const MyComponent = () => {
const { indexPatternEditor } = useAppPlugins();
const { open } = useIndexPatternFlyout(indexPatternEditor, {
onSave: () => {},
});
return (
<button onClick={open}>Add index pattern</button>
)
} |
As best I can tell, we have specific use cases that need the imperative API. Do we have specific use cases that need the declarative api? Otherwise I'm tempted to stick with imperative until its unsuitable for a given situation. What do you think @streamich @flash1293 ? |
Yes, this could be added easily. We can add it when its needed. |
Thanks for working on this, @mattkime! The ability to prepopulate an index pattern name would be pretty useful to provide guidance to the user. Similarly, it would be useful to require the timestamp field because many observability use-cases depend on such a field. Would that be possible? |
@weltenwort I updated the code example with some type interfaces. Let me know what you think. |
That looks like it should cover the requirements I can anticipate. 👍 💭 On a side-note, I wonder if you perceive the |
My 2 cents - a generic validation function is more flexible, but the UX is definitely worse because the creation dialog can't prevent the problem from happening (e.g. by simply omitting "No default time field" option in the dropdown) nor highlight the problematic field in the form. |
Very good point, thank you! |
implemented by #84725 |
Some new text
Parent issue for index pattern creation modal - [https://github.com//issues/84725|https://github.com//issues/84725]
A new IndexPatternEditor plugin will be created which will expose an interface for an index pattern creation flyout. It will be loaded asynchronously to be mindful of bundle size. It will initially be used by Index Pattern Management.
Note - this API is heavily based on the IndexPatternFieldEditor API - [https://github.com/elastic/kibana/tree/master/src/plugins/index_pattern_field_editor|https://github.com/elastic/kibana/tree/master/src/plugins/index_pattern_field_editor].
A convenience function will also be provided for checking permissions -
The text was updated successfully, but these errors were encountered: