-
Notifications
You must be signed in to change notification settings - Fork 5
feat(alarms): Introduce output type and its logic #530
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
c5b42d6
c87f2f4
ca9834b
96df096
888c0a5
973ae07
e034fb0
43ed926
6a34d75
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 |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import { ERROR_SEVERITY_WARNING, LABEL_WIDTH, labels, SECONDARY_LABEL_WIDTH, too | |
| import { AlarmsTrendQueryHandler } from 'datasources/alarms/query-type-handlers/alarms-trend/AlarmsTrendQueryHandler'; | ||
| import { Workspace } from 'core/types'; | ||
| import { FloatingError } from 'core/errors'; | ||
| import { InlineSwitch, Stack } from '@grafana/ui'; | ||
| import { InlineSwitch, Space, Stack } from '@grafana/ui'; | ||
|
|
||
| type Props = { | ||
| query: AlarmsTrendQuery; | ||
|
|
@@ -43,6 +43,7 @@ export function AlarmsTrendQueryEditor({ query, handleQueryChange, datasource }: | |
|
|
||
| return ( | ||
| <> | ||
| <Space v={1} /> | ||
|
Collaborator
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. Do we really need this vertical space? I don't see this space in Assets
Collaborator
Author
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. Yes, in the Assets datasource, there's no gap between the Query type and Group by UI elements in Calibration Forecast. We have the same pattern in Alarms since both datasources have only one query type with an output control (List Assets in Assets datasource, List Alarms in Alarms datasource). As I understand it, related controls are grouped together, while different functional sections are separated by gaps. In the Results datasource, we added a gap after the Output control, which works because both query types have output controls, maintaining consistency. I've added a Space component to create a clear separation between the Query Type and query builder sections in the alarms trend workflow, following the mockup UI design specified in the HLD.
Leaving this comment open, to get @kartheeswaran-ni thoughts. |
||
| <Stack> | ||
| <InlineField | ||
| label={labels.queryBy} | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.
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.
This change is required to have less gap between the query type and output UI controls. This will affect
AlarmsTrendeditor gap between the query type and its query builder. Hence addedSpacefrom thegrafana/uito add appropriate space between the controls in Alarms Trend editor.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.
Can you also add the screenshot of alarm trend editor so we can see the difference?
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.
Alarm Trend editor without

SpaceAlarm Trend editor with

Space