-
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
[Response Ops] [Rule Form] Add Rule Form Flyout v2 #206685
base: main
Are you sure you want to change the base?
Changes from 68 commits
1ef801c
1faf112
4cb5b1d
e5b287a
74f59b7
dd6e167
6dd013d
69eb8fa
74d5339
646af0f
d4c91d3
2d61517
1ffea47
19f2fdc
3e9ebe4
70d4dab
9c4d831
547c4da
c2174d6
4719faf
51e33f9
c25c283
dc8c9ab
114e98a
e3a0e7e
dbc9712
fd7abec
4dbc4f0
b0f977b
4b806ac
13429c1
f557eb2
286721f
1fd34d0
c437919
dbbe03a
f9f82ed
f21566b
0efa925
fb70192
4fd1228
f2e01b9
e6befaf
07ee45a
370a2b6
908d36c
52c6414
aa0287f
9ef99b5
bb09a7d
d182820
118a154
b8a2e33
09d456d
c89a707
565c974
def9107
3f16b58
27cb8b3
688c145
c3eff4f
3ef3181
756cfa8
95b14da
aa0abd0
990a73c
c4a2c9b
3772d57
319f869
99c4dba
9298807
0b991bf
732ce23
4539f97
929ce73
e9948a2
944621a
699300b
1f1f1a5
89f8aaa
0249c2d
0a01b81
42e12ee
abb8326
1013d20
4453609
a8ffcf1
619a37b
850bb6b
acaddb9
3ab44d2
fc004b0
c1edbf3
2d229e6
30c3d46
f8b000b
75f6b91
ce5d6d6
7797540
887b65d
cee9258
c72958b
9e7716d
314d14c
e1fac6f
3b0f4aa
35acdb7
feb0c83
8b13750
9c5b11a
b168e01
30b89de
f4aff0f
6e765ec
c015311
85971c3
587f5c2
5e84003
fd7e080
9e3d7e9
20c0821
78cf8db
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 |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
export * from './confirm_create_rule'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
import React from 'react'; | ||
import { EuiConfirmModal, EuiText } from '@elastic/eui'; | ||
import { | ||
RULE_FORM_CANCEL_MODAL_TITLE, | ||
RULE_FORM_CANCEL_MODAL_CONFIRM, | ||
RULE_FORM_CANCEL_MODAL_CANCEL, | ||
RULE_FORM_CANCEL_MODAL_DESCRIPTION, | ||
} from '../../translations'; | ||
|
||
export interface ConfirmRuleCloseRuleProps { | ||
onCancel: () => void; | ||
onConfirm: () => void; | ||
} | ||
|
||
export const ConfirmRuleClose = (props: ConfirmRuleCloseRuleProps) => { | ||
const { onCancel, onConfirm } = props; | ||
|
||
return ( | ||
<EuiConfirmModal | ||
onCancel={onCancel} | ||
onConfirm={onConfirm} | ||
data-test-subj="confirmRuleCloseModal" | ||
buttonColor="danger" | ||
defaultFocusedButton="confirm" | ||
title={RULE_FORM_CANCEL_MODAL_TITLE} | ||
confirmButtonText={RULE_FORM_CANCEL_MODAL_CONFIRM} | ||
cancelButtonText={RULE_FORM_CANCEL_MODAL_CANCEL} | ||
> | ||
<EuiText> | ||
<p>{RULE_FORM_CANCEL_MODAL_DESCRIPTION}</p> | ||
</EuiText> | ||
</EuiConfirmModal> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
export * from './confirm_rule_close'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
export * from './request_code_block'; | ||
export * from './confirm_create_rule'; | ||
export * from './confirm_rule_close'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,12 @@ import React, { useCallback } from 'react'; | |
import { EuiLoadingElastic } from '@elastic/eui'; | ||
import { toMountPoint } from '@kbn/react-kibana-mount'; | ||
import { type RuleCreationValidConsumer } from '@kbn/rule-data-utils'; | ||
import type { RuleFormData, RuleFormPlugins } from './types'; | ||
import type { RuleFormData, RuleFormPlugins, RuleTypeMetaData } from './types'; | ||
import { DEFAULT_VALID_CONSUMERS, getDefaultFormData } from './constants'; | ||
import { RuleFormStateProvider } from './rule_form_state'; | ||
import { useCreateRule } from './common/hooks'; | ||
import { RulePage } from './rule_page'; | ||
import { RuleFlyout } from './rule_flyout'; | ||
import { | ||
RuleFormCircuitBreakerError, | ||
RuleFormErrorPromptWrapper, | ||
|
@@ -44,9 +45,13 @@ export interface CreateRuleFormProps { | |
shouldUseRuleProducer?: boolean; | ||
canShowConsumerSelection?: boolean; | ||
showMustacheAutocompleteSwitch?: boolean; | ||
isFlyout?: boolean; | ||
isServerless?: boolean; | ||
onCancel?: () => void; | ||
onSubmit?: (ruleId: string) => void; | ||
onChangeMetaData?: (metadata?: RuleTypeMetaData) => void; | ||
initialValues?: Partial<RuleFormData>; | ||
initialMetadata?: RuleTypeMetaData; | ||
} | ||
|
||
export const CreateRuleForm = (props: CreateRuleFormProps) => { | ||
|
@@ -61,9 +66,13 @@ export const CreateRuleForm = (props: CreateRuleFormProps) => { | |
shouldUseRuleProducer = false, | ||
canShowConsumerSelection = true, | ||
showMustacheAutocompleteSwitch = false, | ||
isFlyout, | ||
isServerless = false, | ||
onCancel, | ||
onSubmit, | ||
onChangeMetaData, | ||
initialMetadata, | ||
initialValues = {}, | ||
} = props; | ||
|
||
const { http, docLinks, notifications, ruleTypeRegistry, ...deps } = plugins; | ||
|
@@ -158,24 +167,30 @@ export const CreateRuleForm = (props: CreateRuleFormProps) => { | |
); | ||
} | ||
|
||
const RuleFormUIComponent = isFlyout ? RuleFlyout : RulePage; | ||
|
||
return ( | ||
<div data-test-subj="createRuleForm"> | ||
<RuleFormStateProvider | ||
initialRuleFormState={{ | ||
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. we could take the chance to memo this object to avoid unnecessary rerenders 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.
I'd like to avoid overuse of |
||
formData: getDefaultFormData({ | ||
ruleTypeId, | ||
name: `${ruleType.name} rule`, | ||
consumer: getInitialConsumer({ | ||
consumer, | ||
ruleType, | ||
shouldUseRuleProducer, | ||
}), | ||
schedule: getInitialSchedule({ | ||
ruleType, | ||
minimumScheduleInterval: uiConfig?.minimumScheduleInterval, | ||
formData: { | ||
...getDefaultFormData({ | ||
ruleTypeId, | ||
name: `${ruleType.name} rule`, | ||
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. should "rule" be translated? 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. Yeah probably, didn't catch that in the original code |
||
consumer: getInitialConsumer({ | ||
consumer, | ||
ruleType, | ||
shouldUseRuleProducer, | ||
}), | ||
schedule: getInitialSchedule({ | ||
ruleType, | ||
minimumScheduleInterval: uiConfig?.minimumScheduleInterval, | ||
}), | ||
actions: [], | ||
}), | ||
actions: [], | ||
}), | ||
...initialValues, | ||
}, | ||
metadata: initialMetadata, | ||
plugins, | ||
connectors, | ||
connectorTypes, | ||
|
@@ -201,7 +216,13 @@ export const CreateRuleForm = (props: CreateRuleFormProps) => { | |
}), | ||
}} | ||
> | ||
<RulePage isEdit={false} isSaving={isSaving} onCancel={onCancel} onSave={onSave} /> | ||
<RuleFormUIComponent | ||
isEdit={false} | ||
isSaving={isSaving} | ||
onCancel={onCancel} | ||
onSave={onSave} | ||
onChangeMetaData={onChangeMetaData} | ||
/> | ||
</RuleFormStateProvider> | ||
</div> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,11 @@ | |
import React, { useCallback, useMemo } from 'react'; | ||
import { EuiLoadingElastic } from '@elastic/eui'; | ||
import { toMountPoint } from '@kbn/react-kibana-mount'; | ||
import type { RuleFormData, RuleFormPlugins } from './types'; | ||
import type { RuleFormData, RuleFormPlugins, RuleTypeMetaData } from './types'; | ||
import { RuleFormStateProvider } from './rule_form_state'; | ||
import { useUpdateRule } from './common/hooks'; | ||
import { RulePage } from './rule_page'; | ||
import { RuleFlyout } from './rule_flyout'; | ||
import { RuleFormHealthCheckError } from './rule_form_errors/rule_form_health_check_error'; | ||
import { useLoadDependencies } from './hooks/use_load_dependencies'; | ||
import { | ||
|
@@ -32,8 +33,11 @@ export interface EditRuleFormProps { | |
plugins: RuleFormPlugins; | ||
showMustacheAutocompleteSwitch?: boolean; | ||
connectorFeatureId?: string; | ||
isFlyout?: boolean; | ||
onCancel?: () => void; | ||
onSubmit?: (ruleId: string) => void; | ||
onChangeMetaData?: (metadata?: RuleTypeMetaData) => void; | ||
initialMetadata?: RuleTypeMetaData; | ||
} | ||
|
||
export const EditRuleForm = (props: EditRuleFormProps) => { | ||
|
@@ -44,6 +48,9 @@ export const EditRuleForm = (props: EditRuleFormProps) => { | |
connectorFeatureId = 'alerting', | ||
onCancel, | ||
onSubmit, | ||
isFlyout, | ||
onChangeMetaData, | ||
initialMetadata, | ||
} = props; | ||
const { http, notifications, docLinks, ruleTypeRegistry, application, ...deps } = plugins; | ||
const { toasts } = notifications; | ||
|
@@ -179,6 +186,8 @@ export const EditRuleForm = (props: EditRuleFormProps) => { | |
return action; | ||
}); | ||
|
||
const RuleFormUIComponent = isFlyout ? RuleFlyout : RulePage; | ||
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. Maybe for another PR but shouldn't we take advantage of it and also lazy load the flyout ? It seems like the lazy loading is only done for other plugins. 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. Sure, it's possible we could optimize this rendering step further with some lazy-loading. Maybe with: const RulePage = lazy(() => import('./rule_page).then((module) => ({ default: module.RulePage })));
const RuleFlyout = lazy(() => import('./rule_flyout).then((module) => ({ default: module.RuleFlyout })));
<Suspense fallback={<EuiLoadingElastic size="xl" />}><RuleFormUIComponent></Suspense> I'd like to get this PR out though so if we can open an issue to tackle this that'd be great |
||
|
||
return ( | ||
<div data-test-subj="editRuleForm"> | ||
<RuleFormStateProvider | ||
|
@@ -197,6 +206,7 @@ export const EditRuleForm = (props: EditRuleFormProps) => { | |
actions: actionsWithFrequency, | ||
}, | ||
id, | ||
metadata: initialMetadata, | ||
plugins, | ||
minimumScheduleInterval: uiConfig?.minimumScheduleInterval, | ||
selectedRuleType: ruleType, | ||
|
@@ -211,7 +221,13 @@ export const EditRuleForm = (props: EditRuleFormProps) => { | |
showMustacheAutocompleteSwitch, | ||
}} | ||
> | ||
<RulePage isEdit={true} isSaving={isSaving} onSave={onSave} onCancel={onCancel} /> | ||
<RuleFormUIComponent | ||
isEdit={true} | ||
isSaving={isSaving} | ||
onSave={onSave} | ||
onCancel={onCancel} | ||
onChangeMetaData={onChangeMetaData} | ||
/> | ||
</RuleFormStateProvider> | ||
</div> | ||
); | ||
|
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.
There is a service to dynamically load ECS data for selected fields: https://github.com/elastic/kibana/blob/main/x-pack/platform/plugins/shared/fields_metadata/README.md
Or when rendering it's possible to simply use
<FieldDescription .../>
component from https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-field-utils/src/components/field_description/field_description.tsxThere 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.
will this be addressed in 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.
If it helps for this use case, can be definitely done separately.
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've honestly had to switch this back and forth from dynamic import to static import like 3 times over the past couple months of making changes to
alerts-ui-shared
and the rule form package, everything has been unrelated and just moving other code around, so I haven't had time to actually examine what EcsFlat is used for here. Onmain
Webpack puts it in an asnyc chunk and on this PR for some reason it changes its mind about that. Handling this better is beyond the scope of this PR, I'm just trying to keep the bundle size down so it can merge.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 will open an issue for this. There are other places where we load the ECS package.
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.
Issue: #211585.