-
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
[Alerting] embedding the flyout requires host solutions to manually require alerting deps #61094
Comments
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
Why is this a problem? I think we want to allow for alerting / actions to be disable-able, so having embedders have an optional dependency on alerting seems reasonable. How else would they know alerting is enabled or not, to figure out whether or not to enable alerting flyouts? I guess I'm not sure what without having to change code means ... |
This is in reference the flyout needing to be wrapped in a context provider. Each plugin that wants to use our flyout needs to require and pass in our dependencies (ex: http, docLinks, toastNotifications). Here's an example of what the metrics UI code needs to pass in order to use our flyout: https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/public/components/alerting/metrics/alert_flyout.tsx#L30-L42. |
Ah passing in the deps as parameters! I got it, thx. |
Might be able to do at the same time as #65146? |
@gmmorris @YulNaumenko looks like this issue is resolved by #83248? Since the flyouts can be accessed via the plugin's start contract and no longer requires alerting dependencies. |
Yup, agreed :) |
We should find a way to allow embedding the alerting flyouts without having to change code and manually extend dependencies whenever they're needed in the flyout.
The text was updated successfully, but these errors were encountered: