From cac4b4b443a02fd605ee0e7d99088b5fe5983f97 Mon Sep 17 00:00:00 2001 From: Domas Date: Wed, 21 Jul 2021 18:01:05 +0300 Subject: [PATCH] Alerting: small rule form ux improvements (#36941) * dedupe folder option for existing grafana rules * update test mocks * change toggle to chevron for expanding error state ui * fix some strict lint errors --- .../core/components/Select/FolderPicker.tsx | 16 ++++++----- .../alerting/unified/RuleViewer.test.tsx | 1 + .../unified/components/CollapseToggle.tsx | 7 ++++- .../rule-editor/ExpressionEditor.tsx | 2 +- .../rule-editor/GrafanaConditionsStep.tsx | 27 +++++++++---------- .../components/rule-editor/QueryWrapper.tsx | 2 +- .../unified/components/rules/RulesFilter.tsx | 2 +- .../alerting/unified/utils/amroutes.ts | 2 +- .../alerting/unified/utils/query.test.ts | 1 + .../alerting/unified/utils/rule-form.ts | 2 +- public/app/types/unified-alerting-dto.ts | 1 + 11 files changed, 37 insertions(+), 26 deletions(-) diff --git a/public/app/core/components/Select/FolderPicker.tsx b/public/app/core/components/Select/FolderPicker.tsx index 8212131eb20..51247a1d716 100644 --- a/public/app/core/components/Select/FolderPicker.tsx +++ b/public/app/core/components/Select/FolderPicker.tsx @@ -58,7 +58,7 @@ export class FolderPicker extends PureComponent { }; getOptions = async (query: string) => { - const { rootName, enableReset, initialTitle, permissionLevel, showRoot } = this.props; + const { rootName, enableReset, initialTitle, permissionLevel, initialFolderId, showRoot } = this.props; const params = { query, type: 'dash-folder', @@ -74,8 +74,13 @@ export class FolderPicker extends PureComponent { options.unshift({ label: rootName, value: 0 }); } - if (enableReset && query === '' && initialTitle !== '') { - options.unshift({ label: initialTitle, value: undefined }); + if ( + enableReset && + query === '' && + initialTitle !== '' && + !options.find((option) => option.label === initialTitle) + ) { + options.unshift({ label: initialTitle, value: initialFolderId }); } return options; @@ -122,9 +127,8 @@ export class FolderPicker extends PureComponent { folder = options.find((option) => option.value === initialFolderId) || null; } else if (enableReset && initialTitle) { folder = resetFolder; - } else if (initialTitle && initialFolderId === -1) { - // @TODO temporary, we don't know the id for alerting rule folder in some cases - folder = options.find((option) => option.label === initialTitle) || null; + } else if (initialFolderId) { + folder = options.find((option) => option.id === initialFolderId) || null; } if (!folder && !this.props.allowEmpty) { diff --git a/public/app/features/alerting/unified/RuleViewer.test.tsx b/public/app/features/alerting/unified/RuleViewer.test.tsx index a84471aba02..ad6410cf1ef 100644 --- a/public/app/features/alerting/unified/RuleViewer.test.tsx +++ b/public/app/features/alerting/unified/RuleViewer.test.tsx @@ -87,6 +87,7 @@ const mockGrafanaRule = { grafana_alert: { condition: 'B', exec_err_state: GrafanaAlertStateDecision.Alerting, + namespace_id: 11, namespace_uid: 'namespaceuid123', no_data_state: GrafanaAlertStateDecision.NoData, title: 'Test alert', diff --git a/public/app/features/alerting/unified/components/CollapseToggle.tsx b/public/app/features/alerting/unified/components/CollapseToggle.tsx index 58611eeafd5..dbc83995434 100644 --- a/public/app/features/alerting/unified/components/CollapseToggle.tsx +++ b/public/app/features/alerting/unified/components/CollapseToggle.tsx @@ -7,14 +7,16 @@ interface Props extends HTMLAttributes { onToggle: (isCollapsed: boolean) => void; size?: IconSize; className?: string; + text?: string; } -export const CollapseToggle: FC = ({ isCollapsed, onToggle, className, size = 'xl', ...restOfProps }) => { +export const CollapseToggle: FC = ({ isCollapsed, onToggle, className, text, size = 'xl', ...restOfProps }) => { const styles = useStyles(getStyles); return ( ); }; @@ -26,6 +28,9 @@ export const getStyles = () => ({ outline: none !important; + display: inline-flex; + align-items: center; + svg { margin-bottom: 0; } diff --git a/public/app/features/alerting/unified/components/rule-editor/ExpressionEditor.tsx b/public/app/features/alerting/unified/components/rule-editor/ExpressionEditor.tsx index 95b4938209c..c95961f6e87 100644 --- a/public/app/features/alerting/unified/components/rule-editor/ExpressionEditor.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/ExpressionEditor.tsx @@ -63,7 +63,7 @@ function useQueryMappers(dataSourceName: string): QueryMappers { case 'loki': case 'prometheus': return { - mapToValue: (query: PromQuery | LokiQuery) => query.expr, + mapToValue: (query: DataQuery) => (query as PromQuery | LokiQuery).expr, mapToQuery: (existing: DataQuery, value: string | undefined) => ({ ...existing, expr: value }), }; default: diff --git a/public/app/features/alerting/unified/components/rule-editor/GrafanaConditionsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/GrafanaConditionsStep.tsx index fe811badb37..77f286e11e2 100644 --- a/public/app/features/alerting/unified/components/rule-editor/GrafanaConditionsStep.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/GrafanaConditionsStep.tsx @@ -1,7 +1,7 @@ import React, { FC, useState } from 'react'; import { css } from '@emotion/css'; -import { GrafanaTheme, parseDuration, durationToMilliseconds } from '@grafana/data'; -import { Field, InlineLabel, Input, InputControl, Switch, useStyles } from '@grafana/ui'; +import { parseDuration, durationToMilliseconds, GrafanaTheme2 } from '@grafana/data'; +import { Field, InlineLabel, Input, InputControl, useStyles2 } from '@grafana/ui'; import { useFormContext, RegisterOptions } from 'react-hook-form'; import { RuleFormValues } from '../../types/rule-form'; import { positiveDurationValidationPattern, durationValidationPattern } from '../../utils/time'; @@ -10,6 +10,7 @@ import { GrafanaAlertStatePicker } from './GrafanaAlertStatePicker'; import { RuleEditorSection } from './RuleEditorSection'; import { PreviewRule } from './PreviewRule'; import { GrafanaConditionEvalWarning } from './GrafanaConditionEvalWarning'; +import { CollapseToggle } from '../CollapseToggle'; const MIN_TIME_RANGE_STEP_S = 10; // 10 seconds @@ -43,7 +44,7 @@ const evaluateEveryValidationOptions: RegisterOptions = { }; export const GrafanaConditionsStep: FC = () => { - const styles = useStyles(getStyles); + const styles = useStyles2(getStyles); const [showErrorHandling, setShowErrorHandling] = useState(false); const { register, @@ -83,9 +84,12 @@ export const GrafanaConditionsStep: FC = () => { - - setShowErrorHandling(!showErrorHandling)} /> - + setShowErrorHandling(!collapsed)} + text="Configure no data and error handling" + className={styles.collapseToggle} + /> {showErrorHandling && ( <> @@ -121,7 +125,7 @@ export const GrafanaConditionsStep: FC = () => { ); }; -const getStyles = (theme: GrafanaTheme) => ({ +const getStyles = (theme: GrafanaTheme2) => ({ inlineField: css` margin-bottom: 0; `, @@ -131,12 +135,7 @@ const getStyles = (theme: GrafanaTheme) => ({ justify-content: flex-start; align-items: flex-start; `, - switchField: css` - display: inline-flex; - flex-direction: row-reverse; - margin-top: ${theme.spacing.md}; - & > div:first-child { - margin-left: ${theme.spacing.sm}; - } + collapseToggle: css` + margin: ${theme.spacing(2, 0, 2, -1)}; `, }); diff --git a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx index 15728e61411..eebef6cee0b 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx @@ -63,7 +63,7 @@ export const QueryWrapper: FC = ({ return (
- dataSource={dsSettings} onChangeDataSource={!isExpression ? (settings) => onChangeDataSource(settings, index) : undefined} id={query.refId} diff --git a/public/app/features/alerting/unified/components/rules/RulesFilter.tsx b/public/app/features/alerting/unified/components/rules/RulesFilter.tsx index 66acad4f568..6b3de2b4906 100644 --- a/public/app/features/alerting/unified/components/rules/RulesFilter.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesFilter.tsx @@ -98,7 +98,7 @@ const RulesFilter = () => {
diff --git a/public/app/features/alerting/unified/utils/amroutes.ts b/public/app/features/alerting/unified/utils/amroutes.ts index 86436e2ea8e..f2fd0c0b922 100644 --- a/public/app/features/alerting/unified/utils/amroutes.ts +++ b/public/app/features/alerting/unified/utils/amroutes.ts @@ -9,7 +9,7 @@ import { isUndefined, omitBy } from 'lodash'; const defaultValueAndType: [string, string] = ['', timeOptions[0].value]; const matchersToArrayFieldMatchers = (matchers: Record | undefined, isRegex: boolean): Matcher[] => - Object.entries(matchers ?? {}).reduce( + Object.entries(matchers ?? {}).reduce( (acc, [name, value]) => [ ...acc, { diff --git a/public/app/features/alerting/unified/utils/query.test.ts b/public/app/features/alerting/unified/utils/query.test.ts index 206799e4bfb..2e3de3deed8 100644 --- a/public/app/features/alerting/unified/utils/query.test.ts +++ b/public/app/features/alerting/unified/utils/query.test.ts @@ -78,6 +78,7 @@ describe('alertRuleToQueries', () => { const grafanaAlert = { condition: 'B', exec_err_state: GrafanaAlertStateDecision.Alerting, + namespace_id: 11, namespace_uid: 'namespaceuid123', no_data_state: GrafanaAlertStateDecision.NoData, title: 'Test alert', diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 1fe8e1d2bb3..e5006b801ea 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -106,7 +106,7 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF condition: ga.condition, annotations: listifyLabelsOrAnnotations(rule.annotations), labels: listifyLabelsOrAnnotations(rule.labels), - folder: { title: namespace, id: -1 }, + folder: { title: namespace, id: ga.namespace_id }, }; } else { throw new Error('Unexpected type of rule for grafana rules source'); diff --git a/public/app/types/unified-alerting-dto.ts b/public/app/types/unified-alerting-dto.ts index 6ed43aa6829..17f17aeeb88 100644 --- a/public/app/types/unified-alerting-dto.ts +++ b/public/app/types/unified-alerting-dto.ts @@ -120,6 +120,7 @@ export interface PostableGrafanaRuleDefinition { export interface GrafanaRuleDefinition extends PostableGrafanaRuleDefinition { uid: string; namespace_uid: string; + namespace_id: number; } export interface RulerGrafanaRuleDTO {