Skip to content

[IMPAC-613] Add legend icons to cashflow widgets#387

Merged
xaun merged 6 commits intomaestrano:release/1.5.7from
rheasunshine:improvement/613-legend-icons
Aug 24, 2017
Merged

[IMPAC-613] Add legend icons to cashflow widgets#387
xaun merged 6 commits intomaestrano:release/1.5.7from
rheasunshine:improvement/613-legend-icons

Conversation

@rheasunshine
Copy link
Copy Markdown
Contributor

@xaun xaun self-requested a review August 21, 2017 17:22
@xaun
Copy link
Copy Markdown
Contributor

xaun commented Aug 21, 2017

FYI @cesar-tonnoir, @rheasunshine is working on some changes before I review this. Thanks

@rheasunshine
Copy link
Copy Markdown
Contributor Author

Hey @xaun, ready for review

Copy link
Copy Markdown
Contributor

@xaun xaun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @rheasunshine, just a couple of changes 👍

<i ng-style="{'color': getLegendItemColor(item)}" class="fa {{getLegendItemIcon(item)}}" aria-hidden="true"></i>
<i ng-style="{'color': getLegendItemColor(item)}" class="fa {{getLegendItemCheckBox(item)}}" aria-hidden="true"></i>
<div>
<svg ng-include="getLegendItemIcon(item)" class="legend-item-icon" style="fill: {{getLegendItemColor(item)}};"></svg>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ng-style, I've learnt recently that binding expressions into the style does not work on some versions of IE. So now I try use all the angular directives for this sort of thing.

ng-style="{ 'fill': getLegendItemColor(item) }"

Also while you're here, can you change the class on the line above to use ng-class, as the getLegendItemCheckBox method returns a string of class names, we can apply it directly.

class="fa" ng-class="getLegendItemCheckBox(item)"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

projectedCashLegendIcon: ':default/projected-cash.png'
receivablesLegendIcon: ':default/receivables.png'
plotLineLegendIcon: ':default/plot-line-icon.svg'
areaLegendIcon: ':default/area-icon.svg'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason the cash-projection widget icons cannot be svg agian?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cesar-tonnoir I think we were going to wait for Tristan's review. I can make them svg today so that the decision can be made. There is also the question of whether to use a custom legend for the cash-projection widget like the cash-balance widget.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True @rheasunshine, ok no worries sounds good.

name = this.name
imgSrc = ImpacAssets.get(_.camelCase(name + 'LegendIcon'))
img = "<img src='#{imgSrc}'><br>"
return img + ' ' + name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the templates object here is just to reduce the need to define the boilerplate for each chart type, e.g 'line', 'column' etc.

The configuration you've done here is defining a formatter, so therefore should be apart of the formatters instance method of the Chart class.

But, as this configuration is specific to the cash-projection widget, we can override the formatters instance method from the cash-projection component.

accounts-cash-projection.directive.coffee:

w.format = ->
    ...
    $scope.chart ||= new HighchartsFactory($scope.chartId(), w.content.chart, options)
    # Extend default chart formatters to add custom legend img icon
    defaultFormattersConfig = $scope.chart.formatters()
    $scope.chart.formatters = ->
      angular.merge(defaultFormattersConfig, {
        legend:
          useHTML: true
          labelFormatter: ->
            name = this.name
            img = "<img src='dist/images/#{name.replace(' ', '-').toLowerCase()}.png' width = '55px' height = '25px'><br>"
            console.log 'formatter! ', options
            return img + '  ' + name
      })

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot better. Thanks! Fixed :)

plotOptions :
series:
dataLabels:
useHTML: true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you need the plotOptions here? I have tested and it seems to work the same without it, and I can't find the API for it here: http://api.highcharts.com/highcharts/legend

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Copy Markdown
Contributor

@xaun xaun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @rheasunshine, just remove the ImpacAssets DI I have flagged. @cesar-tonnoir are continuing to merge owner dashboard into 1.5 or is it 1.6 now?

angular
.module('impac.services.highcharts-factory', [])
.factory('HighchartsFactory', ($filter)->
.factory('HighchartsFactory', ($filter, ImpacAssets)->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImpacAssets D.I can be removed

@xaun xaun changed the base branch from master to release/1.5.7 August 24, 2017 10:36
@rheasunshine rheasunshine force-pushed the improvement/613-legend-icons branch from 762fe2a to 6487384 Compare August 24, 2017 11:22
@xaun xaun merged commit fafcead into maestrano:release/1.5.7 Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants