Feature/lmi insights #487

MarcoBgn · 2018-03-09T15:11:53Z

Initial WIP changes for the LMI feature
Cherry-picked Xaun's changes fixing build

…-improvement [IMPAC-741] Lmi widgets improvement

…o feature/lmi-insights

manu-d · 2018-03-09T15:48:08Z

@cesar-tonnoir
Can you have a look at this PR, so that I can open all my PR against maestrano's repo and not Marco's?
Thanks

xaun

My main concern is with the size & duplication of the chart.formatter() overrides across the custom widget templates. Other than that, just some incorrect code from the widget generator, and commented code left in.

xaun · 2018-03-19T12:33:07Z

src/components/widgets/sales-average-purchase-size/sales-average-purchase-size.tmpl.html

+    </div>
+
+    <!-- No data found -->
+    <div ng-if="(isDataFound==false)" common-data-not-found on-display-alerts="onDisplayAlerts()" endpoint="::widget.category" width="::widget.width" />


This is the old implementation of the data-not-found directive. I know the generator created it, but its out of date. I updated this in the generator in v1.6.9 but the lmi feature branch is behind.

The data-not-found is now a modal, so no need to show/hide content on $scope.isDataFound. Replace this directive with: <div ng-show="widget.demoData" common-data-not-found />.

Take a look at cash projection template for example, test with a bolt widget for org with no bolt data.

xaun · 2018-03-19T12:33:52Z

src/components/widgets/sales-average-visit/sales-average-visit.tmpl.html

+    </div>
+
+    <!-- No data found -->
+    <div ng-if="(isDataFound==false)" common-data-not-found on-display-alerts="onDisplayAlerts()" endpoint="::widget.category" width="::widget.width" />


same as above

xaun · 2018-03-19T12:37:28Z

src/components/widgets/sales-customers-engagement/sales-customers-engagement.directive.coffee

+  # == Chart Events Callbacks =====================================================================
+  # Sets the transactions list resources type and displays it
+  # onClickBar = (event) ->
+  #   $scope.isChartDisplayed = false


clean up commented code?

xaun · 2018-03-19T12:37:35Z

src/components/widgets/sales-customers-engagement/sales-customers-engagement.directive.coffee

+
+  # == Directive Events Callbacks =====================================================================
+  # $scope.onButtonBack = () ->
+  #   $scope.isChartDisplayed = true


commented code

xaun · 2018-03-19T13:04:31Z

src/components/widgets/sales-average-purchase-size/sales-average-purchase-size.directive.coffee

+            formatter: ->
+              $filter('mnoCurrency')(this.value, currency, false)
+        xAxis: angular.merge([w.content.chart.xAxis[0]], [xAxisLabels])
+        yAxis: angular.merge([w.content.chart.yAxis[0]], [yAxisLabels])


The yAxisLabels are the same as default (right?), for overriding the formatters method, I suggest leveraging the formatters method's returned defaults by savings them to a variable in the outer scope.

Also, rather than having to merge the new v2 bolt layouts response options (xAxis, yAxis, tooltip etc) for every single custom widget template, the highcharts factory should be extended to use these by default.

Then you could do this sort of thing:

defaultFormatters = $scope.chart.formatters() $scope.chart.formatters = -> defaultFormatters.labels.formatter = -> formatString = if this.chart.rangeSelector.options.selected >= 3 then 'MMM YYY' else 'DD MMM' moment.utc(this.value).format(formatString) defaultFormatters.rangeSelector = { selected: 5 } defaultFormatters

EDIT: so I can see now that you have extended the highcharts formatter, so would simply be saving the defaultFormatters be enough to not have to do the angular.merge([w.content.chart.xAxis[0] .. ) ? Seems like you are giving the formatter the w.content.chart.xAxis in initialization, but then having to re-provide it with it to merge in the custom opts.

@xaun I am not completely sure about this as there seem to be slight differences in the overridden formatters

@MarcoBgn Im not completely across the intricacies of your customisations, so my solution isn't comprehensive for your use-case. I am just showing you a way you can store the existing defaults, then modify the object and reassign the formatters method.

The main thing is, this method is massive in the widget controller, it redefines some values that are already defined in the default, and looks very similar across all of the new widgets. So for my review, I request that we think of a solution to improve this. If this is not in the scope of this body of work, or it needs to be quick & dirty merged or something, that's another story :)

xaun · 2018-03-19T13:05:41Z

src/components/widgets/sales-customers-engagement/sales-customers-engagement.tmpl.html

+  <!-- Content Panel -->
+  <div ng-hide="widget.isEditMode">
+    <!-- Data found -->
+    <div ng-show="isChartDisplayed" class="">


Does isChartDisplayed ever get set false?

@manu-d as discussed could you check this?

xaun · 2018-03-19T13:09:18Z

src/services/highcharts-theme/highcharts-theme.svc.coffee

@@ -0,0 +1,32 @@
+angular
+.module('impac.services.highcharts-theme', [])
+.service('HighchartsThemeService', (ImpacTheming) ->


Looks more like something that should live in an angular .run() function?

I am having issues with Highcharts.theme = where Highcharts is not defined yet. Not being a service (i.e. no HighcartsProvider), I am not sure this can live in a run() function

xaun · 2018-03-19T13:17:43Z

src/services/highcharts-factory/highcharts-factory.svc.coffee

@@ -63,7 +69,7 @@ angular
      return @

    template: ->
-      @_template.get(@data.series, @options)
+      @_template.get(@data.series, @options, @data.xAxis, @data.yAxis, @data.plotOptions)


Ok for now, @cesar-tonnoir will latest 1.6 be back-merged into lmi? If so there will be lots of conflicts as this whole factory has been reworked (#480) to simplify providing options.

@xaun I don't think that will be necessary: the LMI widgets will use the figure+chart generic template (will be developed in 1.8 with your implementation of generic templates loading), so ultimately we shouldn't need that many customisations

@cesar-tonnoir when you say "will be developed", is that different to merging this branch into upstream/1.x ? Seems there is quite a bit of HighchartsFactory#formatter customisation (of which I am not too happy with the resulting code), will the formatter stuff be discarded for generic templates? Or will it live separately somehow?

xaun

Im not really sure about whether these works are going to be merged into upstream branches. From @cesar-tonnoir's previous comment it looks like lmi widgets are going to use generic templates? So does that mean these widget controllers are going to be retired? If not, my review is that the formatters methods need refactoring.

MarcoBgn and others added 12 commits November 16, 2017 13:21

add sales-average-purchase-size Widget

35c82af

Custom template for Highchart-LMI + Widgets

da4141a

Remove double controller

3c4ba45

WIP info panel

d9aed2b

Add customer engagement widget, demo LMI customisation

490eddb

[IMPAC-741] Add $ to right axis

3faa8df

[IMPAC-741] Disable zooming for average visit

52ae0bd

[IMPAC-741] Add margin to legend

3e3bd78

[IMPAC-741] Standardize navigator line to straight dashed

c3014b2

Merge pull request #1 from iseessel/improvement/741-impac-lmi-widgets…

6aa4439

…-improvement [IMPAC-741] Lmi widgets improvement

Solve merge conflicts

6755f5a

Merge branch 'lmi-insights' of github.com:maestrano/impac-angular int…

c32815e

…o feature/lmi-insights

manu-d requested a review from cesar-tonnoir March 9, 2018 15:46

manu-d assigned cesar-tonnoir Mar 9, 2018

MarcoBgn and others added 4 commits March 9, 2018 15:49

Remove Angular Scenario breaking JQuery

8641af1

Get Impac Theming colors for Highcharts

6e210e0

Merge branch 'fix/travis-chromelauncher' into fix/dependencies-and-specs

7902a41

Fix dashboard specs (relating to IMPAC-750)

7616571

xaun suggested changes Mar 19, 2018

View reviewed changes

MarcoBgn requested a review from xaun March 28, 2018 10:18

changes after review

00b562f

MarcoBgn force-pushed the feature/lmi-insights branch from 1d8d979 to 00b562f Compare March 28, 2018 12:05

xaun suggested changes Apr 3, 2018

View reviewed changes

cesar-tonnoir mentioned this pull request Apr 11, 2018

Feature/lmi bolt #446

Closed

cesar-tonnoir added this to the v1.8.0 milestone Apr 11, 2018

cesar-tonnoir added review needed enhancement labels Apr 11, 2018

cesar-tonnoir modified the milestones: v1.8.0, v1.9.0 May 24, 2018

agranado2k modified the milestones: v1.9.0, LMI Jun 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/lmi insights #487

Feature/lmi insights #487

MarcoBgn commented Mar 9, 2018 •

edited

Loading

manu-d commented Mar 9, 2018

xaun left a comment •

edited

Loading

xaun Mar 19, 2018

xaun Mar 19, 2018

xaun Mar 19, 2018

xaun Mar 19, 2018

xaun Mar 19, 2018

MarcoBgn Mar 28, 2018

xaun Apr 3, 2018

xaun Mar 19, 2018

MarcoBgn Mar 28, 2018

xaun Apr 3, 2018

xaun Mar 19, 2018

MarcoBgn Mar 28, 2018

xaun Mar 19, 2018

cesar-tonnoir Mar 20, 2018

xaun Apr 3, 2018

xaun left a comment

Feature/lmi insights #487

Are you sure you want to change the base?

Feature/lmi insights #487

Conversation

MarcoBgn commented Mar 9, 2018 • edited Loading

manu-d commented Mar 9, 2018

xaun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaun left a comment

Choose a reason for hiding this comment

MarcoBgn commented Mar 9, 2018 •

edited

Loading

xaun left a comment •

edited

Loading