[IMPAC-622] threshold tooltip#393
[IMPAC-622] threshold tooltip#393xaun wants to merge 7 commits intomaestrano:release/v1.6.1from xaun:feature-1.6.1/622-threshold-tooltip
Conversation
|
@cesar-tonnoir I have added commits to:
|
cesar-tonnoir
left a comment
cesar-tonnoir
left a comment
There was a problem hiding this comment.
@xaun
I've tried to refactor this branch and gave up after ~3 hours: the way the thresholds are added to the chart is too complex and the code is too intricate. Even if the feature seems to work, if I was to merge this branch, it would add yet another layer of complexity on something that should already be refactored.
So, before going further with the tooltips, I would like you to refactor the thresholds, taking several things into account:
-
The widget (or the threshold component) should trigger the action to attach a threshold to a Chart. Calling
addTresholds()after the chart initialization, exiting if no threshold option has been set, is not the proper way to go. -
Don't put specific code in the factory: anything related to cash, projection, etc.. even the threshold series name, should be passed as arguments. Otherwise, you are building a factory that works for a single widget.
-
The only major change since you've worked on the thresholds / tooltips is the way we handle the dates with unix timestamps. I can help you with this part, or you can stub the data and let me update it: I'm happy to give a hand on this part as long as the rest of the code is clean, with separated concerns.
Eg:
# Not ok:
constructor: (@id, @data = {}, @options = {})->
@hc = Highcharts.stockChart(@id, chartConfig)
@addThresholds()
addThresholds: (options = @options)->
for threshold in options.thresholds
...
# ok:
# in widget / threshold component:
chart.addThreshold( {...}, cashProjectionSeries)
# thresholdOptions: { name, value, kpiId, color, isFullLength, withTooltip }
addThreshold: (thresholdOptions, targetSeries) ->
...
s = @hc.series.add(myThresholdSeries)
addThresholdTooltip(s, targetSeries) if thresholdOptions.withTooltip| return removePointDataLabels(@triggeredPoint) unless thresholdSerie | ||
| cashSerie = _.find(chart.series, (s)-> s.name.toLowerCase() == 'projected cash') | ||
|
|
||
| return removePointDataLabels(@triggeredPoint) unless thresholdSerie.options.triggered |
There was a problem hiding this comment.
thresholdSerie.options.triggered always true
| s.options.triggered && s.name.toLowerCase() == 'threshold kpi' | ||
| ) | ||
| return removePointDataLabels(@triggeredPoint) unless thresholdSerie | ||
| cashSerie = _.find(chart.series, (s)-> s.name.toLowerCase() == 'projected cash') |
There was a problem hiding this comment.
code specific to cash projection widget in the chart factory
| else | ||
| @hc.update(chartConfig) | ||
| @addThresholds() | ||
| @addThresholds(@hc) |
There was a problem hiding this comment.
addThresholds and renderThresholdTooltip should be called from the widget context
| labelText = "The cash projection reaches the threshold on #{triggeredIntervalDate}" | ||
| # The first cash projection interval point below the threshold | ||
| @triggeredPoint = cashSerie.points[thresholdSerie.options.triggered_interval_index] | ||
| # Updating dataLabels have no animation, adds delay to improve UI. |
There was a problem hiding this comment.
Not very good.. can't you simply update the chart once the dataLabels object is ready?
| # Remove existing thresholds | ||
| _.each(@hc.series, (s)-> s.remove() if s.name.toLowerCase().includes('threshold')) | ||
| return @hc if _.isEmpty(options.thresholds) | ||
| _.each(chart.series, (s)-> s.remove() if s.name.toLowerCase().includes('threshold')) |
There was a problem hiding this comment.
Do we have to remove all the existing threshold series? What if we want to define several thresholds on a chart in the future?
|
|
||
| # Render a threshold serie dataLabel when KPI triggered | ||
| renderThresholdTooltip: (chart, options = @options)-> | ||
| thresholdSerie = _.find(chart.series, (s)-> |
There was a problem hiding this comment.
should be passed as an argument
| thresholdSerie = _.find(chart.series, (s)-> | ||
| s.options.triggered && s.name.toLowerCase() == 'threshold kpi' | ||
| ) | ||
| return removePointDataLabels(@triggeredPoint) unless thresholdSerie |
There was a problem hiding this comment.
Are you sure we should have @triggeredPoint as a global variable? shouldn't it remain in the method's scope?
| @getApiV2KpiDataKey = (kpi)-> | ||
| # Formats the kpi endpoint to select the key name | ||
| # e.g response.cash_projection = { triggered: true, ... } | ||
| # TODO: maybe the 'kpi' of endpoint 'kpis/cash_projection' should be removed? |
There was a problem hiding this comment.
Yes: I don't think the endpoint was returning kpis/something in the API v1?
Can you please align the Bolt endpoint?
2 participants
@cesar-tonnoir here is the threshold tooltip rebased on top of v.1.6.1. I have applied your refactors as well.
I could not assign the switch like you wanted to in the kpi.svc:
As one of the cases
return $q.reject(err)