[IMPAC-666] Refactor thresholds#407
[IMPAC-666] Refactor thresholds#407xaun merged 2 commits intomaestrano:stagingfrom xaun:refactor-threshold
Conversation
| buildThresholdsFromKpis = -> | ||
| targets = ctrl.widget.kpis? && ctrl.widget.kpis[0] && ctrl.widget.kpis[0].targets | ||
| return [] unless ImpacKpisSvc.validateKpiTargets(targets) | ||
| [{ kpiId: ctrl.widget.kpis[0].id, value: targets.threshold[0].min, name: 'Alert Threshold', color: ctrl.thresholdColor, onClickEvent: onThresholdClick }] |
There was a problem hiding this comment.
@cesar-tonnoir I have left the threshold name static for now as I don't think it makes sense for the widget controller to set this (how would that be if / when this is extended to iterate over multiple widget kpis?), and the UI/UX requirements for the name displayed have not been defined as of yet.
|
Firefox is timing out on the build, any ideas? I cannot reproduce this on local. Looking online and not too sure either. |
cesar-tonnoir
left a comment
cesar-tonnoir
left a comment
There was a problem hiding this comment.
I like it. I like it a lot.
ctrl.chart.addThreshold... ctrl.chart.removeThreshold... sounds like a toolbox :)
LGTM
| ) | ||
| return unless validateHistParameters() | ||
| Highcharts.addEvent(chart.hc.container, 'click', onChartClick) | ||
| _.each buildThresholdsFromKpis(), (threshold)-> |
| buildThresholdsFromKpis = -> | ||
| targets = ctrl.widget.kpis? && ctrl.widget.kpis[0] && ctrl.widget.kpis[0].targets | ||
| return [] unless ImpacKpisSvc.validateKpiTargets(targets) | ||
| [{ kpiId: ctrl.widget.kpis[0].id, value: targets.threshold[0].min, name: 'Alert Threshold', color: ctrl.thresholdColor, onClickEvent: onThresholdClick }] |
|
Regarding the build, this may be because of the changes done on KpisSvc? |
|
@cesar-tonnoir please review my latest commit fixing bug discovered during stress-testing. I have stress tested with this commit & working well. |
| Highcharts.addEvent(chart.hc.container, 'click', onChartClick) | ||
| _.each buildThresholdsFromKpis(), (threshold)-> | ||
| ctrl.chart.addThreshold(threshold) | ||
| thresholdSerie = ctrl.chart.findThreshold(threshold.kpiId) |
There was a problem hiding this comment.
I wouldn't use the kpi id as the key here: couldn't we in the future add thresholds to a chart without them be related to a kpi?
Maybe use the series name instead?
| thresholdSerie | ||
| @hc.addSeries(threshold) | ||
|
|
||
| removeThreshold: (kpiId)-> |
There was a problem hiding this comment.
same here: would it work if you were to use the series name instead of the kpi id?
2 participants
@cesar-tonnoir please review this refactor for the thresholds. Note this is excluding the threshold tooltips as I am experiencing problems with this and stock charts, but wanted to get at least this refactor finished today.
This refactor is linked to your comments made here: #393