Add Alerts Card #329
Add Alerts Card #329
Conversation
Pull Request Test Coverage Report for Build 1356
💛 - Coveralls |
rawagner
force-pushed
the
dashboard_alerts
branch
from
c92b942 to
2fc3b83
Compare
| import { AlertsBody } from '../../Dashboard/Alert/AlertsBody'; | ||
| import { filterAlerts } from './utils'; | ||
|
|
||
| export const Alerts = ({ alertsResponse }) => { |
There was a problem hiding this comment.
can we return null from this component instead?
| export const AlertItem = ({ alert }) => ( | ||
| <Row className="kubevirt-alert__item"> | ||
| <Col lg={1} md={1} sm={1} xs={1}> | ||
| {getSeverityIcon(get(alert, 'labels.severity'))} |
There was a problem hiding this comment.
can we make selectors for alerts?
There was a problem hiding this comment.
added alerts selectors to prometheus folder
|
|
||
| export const AlertItem = ({ alert }) => ( | ||
| <Row className="kubevirt-alert__item"> | ||
| <Col lg={1} md={1} sm={1} xs={1}> |
There was a problem hiding this comment.
can't we rather use css with a margin here?
| <GridItem span={6}> | ||
| <ComplianceConnected /> | ||
| <GridItem span={12}> | ||
| <div className="kubevirt-cluster-overview__health-combined"> |
There was a problem hiding this comment.
wouldn't using another Grid + GridItem's be simpler here?
There was a problem hiding this comment.
right :) using Grid and GridItem now
|
|
||
| export const AlertsBody = ({ alerts }) => ( | ||
| <div className="kubevirt-alert__alerts-body"> | ||
| {alerts.map((alert, index) => ( |
There was a problem hiding this comment.
can we render the alerts as children (AlertItem) for better reusability?
| }; | ||
|
|
||
| Alerts.propTypes = { | ||
| alertsResponse: PropTypes.oneOfType([PropTypes.array, PropTypes.object]), |
There was a problem hiding this comment.
can we rather send an empty array from web-ui ? This would make things a bit cleaner here
There was a problem hiding this comment.
Im not sure, even though there is no Not Available state yet I think that in the future we will want to notify user that AlertManager is not available/accessible and object as alertsResponse will serve its purpose.
|
Can you please also add alerts to ClusteOverview fixture? |
rawagner
force-pushed
the
dashboard_alerts
branch
from
2fc3b83 to
3e36930
Compare
kubevirt-bot
added
the
needs-rebase
rawagner
force-pushed
the
dashboard_alerts
branch
from
3e36930 to
a89f059
Compare
kubevirt-bot
removed
the
needs-rebase
added |
| }; | ||
|
|
||
| Alerts.propTypes = { | ||
| alertsResponse: PropTypes.oneOfType([PropTypes.array, PropTypes.object]), |
| const alerts = filterAlerts(alertsResponse); | ||
|
|
||
| return alerts.length > 0 ? ( | ||
| <DashboardCard className="kubevirt-alerts__card"> |
There was a problem hiding this comment.
the top border probably shouldn't go to Alerts card
There was a problem hiding this comment.
Im passing className to Alerts from ClusterOverview now
rawagner
force-pushed
the
dashboard_alerts
branch
from
a89f059 to
6d4c96e
Compare
According to design we should have a dropdown which would allow filtering alerts by severity. Severity is defined by labels and label values can be anything so its up to alert creator to set the key and value. For this reason Im not adding dropdown yet. I try to recognize critical alerts by looking for label
severity: critical. All others are evaluated aswarningseverity