-
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?
Conversation
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
b26ee4c to
c87f2f4
Compare
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
|
|
||
| return ( | ||
| <Stack direction='column'> | ||
| <> |
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 AlarmsTrend editor gap between the query type and its query builder. Hence added Space from the grafana/ui to 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.
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
Signed-off-by: Ahalya Radhakrishnan <[email protected]>
| } | ||
|
|
||
| export enum OutputType { | ||
| Properties = 'Properties', |
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.
Other enums are using camelCase. Why do we need sentence case here?
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 have used Pascal case (TotalCount), not camelCase (totalCount), to match other UI oriented enums like QueryType.
|
|
||
| return ( | ||
| <Stack direction='column'> | ||
| <> |
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?
src/datasources/alarms/components/editors/list-alarms/ListAlarmsQueryEditor.tsx
Show resolved
Hide resolved
src/datasources/alarms/components/editors/list-alarms/ListAlarmsQueryEditor.test.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| return ( | ||
| <> | ||
| <Space v={1} /> |
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.
Do we really need this vertical space? I don't see this space in Assets
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.
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.
|
Please update the description to explain why we need this change and also link the PR where the alarm count code will be removed |
src/datasources/alarms/query-type-handlers/list-alarms/ListAlarmsQueryHandler.ts
Outdated
Show resolved
Hide resolved
src/datasources/alarms/constants/AlarmsQueryEditor.constants.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Ahalya Radhakrishnan <[email protected]>


Pull Request
🤨 Rationale
Task 3513143: PR1 | Add Output Radio Group and its logic
This PR introduces output type control and its logic. This change is necessary because alarms count should be treated as an output type option (displaying properties vs. displaying total count) rather than a separate query type. This approach aligns with other datasources like Assets, Results, Test Plans, and Work Orders, which use total count as an output type to display the count.
A separate PR has been created to remove the alarms count query type and its related code throughout the alarms datasource. Here is the link to that PR.
👩💻 Implementation
ListAlarms.types.ts.ListAlarmsQueryEditor. Added in on change method to update the output type in the query.ListAlarmsQueryEditorto show or hide the UI controls based on the output type selection.ListAlarmsQueryHandler, I have refactored therunQueryby introducing private methods to handle the total count and properties queries separately. This makes the code cleaner and easier to debug.queryAlarmswith the required fields for total count fromAlarmsCountQueryHandler.🧪 Testing
Added unit tests
✅ Checklist