-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(alarms): add alarm resource explorer #2993
Conversation
a7c6948
to
9bfa678
Compare
4891063
to
57a355f
Compare
Can you add some screenshots for the alarm table + showing the latest alarm data? |
...nents/src/components/resource-explorers/explorers/alarm-explorer/internal-alarm-explorer.tsx
Show resolved
Hide resolved
} | ||
|
||
try { | ||
const { stateName } = JSON.parse(alarmStateJSON); |
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.
Let's use a type-safe JSON parse with a cast to an internal type that matches the expected state value
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.
Is there a library we are using for this? Otherwise I can make a safeParse function
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 don't know of any good external libraries, we should write one if it's not too much of a headache
if (ALARM_STATUS[stateName as UpperCaseStateName] != null) { | ||
normalizedStateName = ALARM_STATUS[stateName as UpperCaseStateName]; | ||
} else { | ||
normalizedStateName = stateName; |
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.
Why are we setting an unknown state? We should probably log/throw an error
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 actually copied this from the existing alarms functionality in source-iotsitewise. I had assumed there was some legacy complication that we wanted to be able to display something atleast
...nents/src/components/resource-explorers/explorers/alarm-explorer/internal-alarm-explorer.tsx
Outdated
Show resolved
Hide resolved
...-components/src/components/resource-explorers/explorers/alarm-explorer/transformAlarmData.ts
Show resolved
Hide resolved
...-components/src/components/resource-explorers/explorers/alarm-explorer/transformAlarmData.ts
Show resolved
Hide resolved
57a355f
to
ed8b9db
Compare
assetCompositeModelId: compositeModelId, | ||
assetId, | ||
assetModelId, | ||
sourcePropertyId: source?.property.id, |
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 should be inputPropertyName
--> alarmData.inputProperty[0].name
We don't need to show the alarm source propertyId
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.
oh I see, this is the wrong "source". haha.
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.
Yeah this is exactly why I renamed "sourceProperty" to "inputProperty" cause it's easy to mess that up :)
ed8b9db
to
afa319a
Compare
The merge-base changed after approval.
afa319a
to
2004b44
Compare
Overview
Adds an Alarm resource explorer which uses the useAlarms hook to display a table of alarms from either asset or asset model parameters.
Note: Some of the tests for the explorers are commented out. They will be revisited in a following PR once the asset property values hooks part useAlarms are completed and stable.
Legal
This project is available under the Apache 2.0 License.