-
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 UI] Replaced AlertsContextProvider with KibanaContextProvider and exposed components in API #84604
Changes from 11 commits
fc8a25a
e868dc5
31bfbf1
1d23be2
019e223
0702d63
98a0baf
a8f19c5
3147ea8
cc5f649
f3a86ff
3c7e302
a8c0f57
eb1e611
c8e249e
9097aea
74d3b5d
4ddc524
7972208
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,29 +3,38 @@ | |
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
import React from 'react'; | ||
import React, { useMemo } from 'react'; | ||
import { useKibana } from '../../../../../../../src/plugins/kibana_react/public'; | ||
import { AlertType } from '../../../../common/alert_types'; | ||
import { AlertAdd } from '../../../../../triggers_actions_ui/public'; | ||
|
||
type AlertAddProps = React.ComponentProps<typeof AlertAdd>; | ||
import { TriggersAndActionsUIPublicPluginStart } from '../../../../../triggers_actions_ui/public'; | ||
|
||
interface Props { | ||
addFlyoutVisible: AlertAddProps['addFlyoutVisible']; | ||
setAddFlyoutVisibility: AlertAddProps['setAddFlyoutVisibility']; | ||
addFlyoutVisible: boolean; | ||
setAddFlyoutVisibility: React.Dispatch<React.SetStateAction<boolean>>; | ||
alertType: AlertType | null; | ||
} | ||
|
||
interface KibanaDeps { | ||
triggersActionsUi: TriggersAndActionsUIPublicPluginStart; | ||
} | ||
|
||
export function AlertingFlyout(props: Props) { | ||
const { addFlyoutVisible, setAddFlyoutVisibility, alertType } = props; | ||
return ( | ||
alertType && ( | ||
<AlertAdd | ||
addFlyoutVisible={addFlyoutVisible} | ||
setAddFlyoutVisibility={setAddFlyoutVisibility} | ||
consumer="apm" | ||
alertTypeId={alertType} | ||
canChangeTrigger={false} | ||
/> | ||
) | ||
const { | ||
services: { triggersActionsUi }, | ||
} = useKibana<KibanaDeps>(); | ||
const AddAlertFlyout = useMemo( | ||
() => | ||
alertType && | ||
triggersActionsUi.getAddAlertFlyout({ | ||
consumer: 'apm', | ||
addFlyoutVisible, | ||
setAddFlyoutVisibility, | ||
alertTypeId: alertType, | ||
canChangeTrigger: false, | ||
}), | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[addFlyoutVisible, alertType] | ||
); | ||
return <>{AddAlertFlyout}</>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why this cannot be a component that the alerting UI exports? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the code reducing prospective it sounds right to expose it as component, but our decision was to live the details of the flyout usage to the end plugin - as a result it can be without memoizations or with the own dependancies list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the memoization is not needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks ok, but since it's not a component could we make the variable lowercase? Also why are we disabling exhaustive-deps? If there's a good reason for that please add a comment explaining why. |
||
} |
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.
Ideally
startDeps
is part of the plugin context value, similar to plugins. We've not named the properties here appropriately but we'll change that separately from this PR.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.
Sounds right for me, I had the similar thoughts, but decided not to modify your plugin context. Are you OK to have it for this PR as it is currently implemented?
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.
Sure thing.