[IMPAC-648] v1.6.0 - Refactor "data not found" mode by cesar-tonnoir · Pull Request #385 · maestrano/impac-angular

cesar-tonnoir · 2017-08-14T01:22:33Z

Better styling and wording
Example mode with stub data
Update delete widget 'pop-up' to match the new design
Slight refactor of WidgetSvc

cesar-tonnoir · 2017-08-15T15:17:39Z

@xaun this is ready - can you please review?
goes with: https://github.com/maestrano/impac/pull/486

+ improve styling and messaging + remove background images from template + remove myob custom message

+ handle widget.isLoading in the service + #show to call itself again with `demo=true` when no data found + refactor returned promises

xaun

Looks good, just requesting changes for new components to be written as a .component

xaun · 2017-08-17T10:01:43Z

@@ -0,0 +1,20 @@
+module = angular.module('impac.components.common.delete-widget',[])
+
+module.directive('commonDeleteWidget', ($templateCache, ImpacWidgetsSvc, ImpacUtilities) ->


For new components, we should be leveraging the angular .component.

bindings: { # one-way binding parentWidget: '<' } ... # lifecycle hooks ctrl.$ngOnInit = -> ctrl.loading = false

.directives should really only be used now to attach functionality onto an element. For example the impac-angular/src/components/widgets-common/autofocus/autofocus.directive.coffee directive.

xaun · 2017-08-17T10:08:07Z

+
+      scope.deleteWidget = ->
+        scope.loading = true
+        ImpacWidgetsSvc.delete(scope.parentWidget)


It should be the parents (impacWidget) responsibility to make the backend request to delete the widget. This component should just emit an event via an onDelete callback.

I don't completely agree with this one: as the component is called "delete-widget", one can expect it is going to actually delete the widget by calling the WidgetsSvc... Otherwise, you always put all the logic in a single "widget" component.

I'll apply your suggestion, we can always revert later if needed.

@cesar-tonnoir I think @alexnoox agrees with me judging by the reaction.. I was going to ask for his input. The delete-widget should just be a presentational component. If you apply what you have done to all components, you will find that your codebase makes requests to APIs from all sorts of places. It seems logically for it to encapsulate the request as it's purpose it to delete. But it actually makes much more sense for the widget.component to be the "smart component", and for it to make the request to the widget service.

Also, think about having many components like this, you will then have the WidgetsSvc dependency across many different places, and it becomes hard to follow.

You can read up on this design pattern by searching "smart components dumb components" or "smart components presentational components" (e.g http://blog.angular-university.io/angular-2-smart-components-vs-presentation-components-whats-the-difference-when-to-use-each-and-why/).

xaun · 2017-08-17T10:32:22Z

+
+    # Calls Legacy Impac! or bolt API to render a widget
+    # If no content is returned, the endpoint will be called again with `demo` = `true` to retrieve stub data
+    @show = (widget, refreshCache = false, demo = false) ->


Maybe the demo flag should be within an opts arg instead? More flexible with future additions.

xaun · 2017-08-17T10:38:20Z

      'bower_components/angular-xeditable/dist/js/xeditable.js',
      'bower_components/angular-toastr/dist/angular-toastr.tpls.js',
      'bower_components/bootstrap/dist/js/bootstrap.js',
+      'bower_components/ng-tags-input/ng-tags-input.js',


Not sure this needs to be commited?

I think this should have been part of v1.5.6, it keeps appearing every time I run the specs...
As all the other bower_components are committed, wouldn't it be more consistent to add this one as well?

Yeah if it is a bower_component then it should be committed. But as I cannot see it in the bower.json I thought it was a mistake. I just tested though and it's writing it to my karma files on gulp test too.. Must be a sub-dep

xaun · 2017-08-17T10:38:25Z

      'bower_components/angular-xeditable/dist/js/xeditable.js',
      'bower_components/angular-toastr/dist/angular-toastr.tpls.js',
      'bower_components/bootstrap/dist/js/bootstrap.js',
+      'bower_components/ng-tags-input/ng-tags-input.js',


xaun · 2017-08-17T10:42:16Z

-                (updatedWidget) ->
-                  _self.show(updatedWidget).finally( -> updatedWidget.isLoading = false )
-              )
+              promises.push _self.update(wgt, { metadata: wgt.metadata })


Is the .then((updatedWidget)-> updatedWidget.isLoading = false not needed anymore?

No, that's an improvement: the loader is now fully managed by the #show and #update methods in the service.
We don't need to care about it in the controllers or other methods anymore.

cesar-tonnoir · 2017-08-18T00:56:51Z

@xaun thanks for the review. I've applied your suggestions.
I'm going to merge the PR as this needs to be deployed tonight. Please let me know if you have any other comment.

xaun · 2017-08-23T13:13:23Z

@cesar-tonnoir In my review I forgot to highlight the fact that a config change was made & the README.md configurations section(s) were not updated. Do you think you could patch this? Cheers

cesar-tonnoir added WIP enhancement and removed WIP labels Aug 14, 2017

cesar-tonnoir requested a review from xaun August 15, 2017 15:17

cesar-tonnoir changed the base branch from release/1.5.6 to 1.6 August 15, 2017 15:29

cesar-tonnoir changed the base branch from 1.6 to release/1.5.6 August 15, 2017 15:31

cesar-tonnoir changed the base branch from release/1.5.6 to 1.6 August 16, 2017 04:28

cesar-tonnoir changed the title ~~Refactor "data not found" mode~~ [IMPAC-648] v1.6.0 - Refactor "data not found" mode Aug 16, 2017

cesar-tonnoir force-pushed the feature/demo-data branch from e355ebb to d364f57 Compare August 16, 2017 04:30

cesar-tonnoir added 19 commits August 16, 2017 14:31

rework data-not-found component

df5e9a0

+ improve styling and messaging + remove background images from template + remove myob custom message

widget to fetch demo data when no data found

9303203

widget deletion popup to match new design

35ec45e

remove deprecated data-not-found fallback images

f637704

remove isDataFound from widgets templates

0ea61ec

move delete-widget popup to separate directive

07e4d31

change demo data "add app" link color

d6ecd89

fix delete-widget overlay

69a664a

add data-not-found to custom calc widget

ab2a851

refactor WidgetsSvc #show and #update

1c95e89

+ handle widget.isLoading in the service + #show to call itself again with `demo=true` when no data found + refactor returned promises

fix widget.isHistory initialisation

0640df7

balance sheet: add data-not-found directive

6de6a61

fix expense-weight widget

8c39bd5

hide "data not found" in designer mode

ac3ea48

remove "new" badge from KPIs bar

ec365a4

fix sales forecast css glitch

a56639c

load stubbed data only once

732369b

update changelog

0a0d425

build

461fb15

cesar-tonnoir force-pushed the feature/demo-data branch from d364f57 to 461fb15 Compare August 16, 2017 04:33

cesar-tonnoir added 2 commits August 16, 2017 16:24

fix widgets svc specs

bd6f88e

remove container width override in workspace

ea09153

xaun suggested changes Aug 17, 2017

View reviewed changes

cesar-tonnoir added 4 commits August 18, 2017 10:32

transform delete-widget from directive to component

ec9496c

use callback instead of parentWidget object for delete-widget comp.

adb382d

pass options hash to WidgetsSvc#show

4f61c7f

build

4299fe0

cesar-tonnoir merged commit fd4c6f1 into maestrano:1.6 Aug 18, 2017

cesar-tonnoir deleted the feature/demo-data branch November 6, 2017 23:54

		@@ -0,0 +1,20 @@
		module = angular.module('impac.components.common.delete-widget',[])

		module.directive('commonDeleteWidget', ($templateCache, ImpacWidgetsSvc, ImpacUtilities) ->

Conversation

cesar-tonnoir commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cesar-tonnoir commented Aug 15, 2017

Uh oh!

xaun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesar-tonnoir commented Aug 18, 2017

Uh oh!

xaun commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cesar-tonnoir commented Aug 14, 2017 •

edited

Loading