Convert wizard directive #377
Conversation
| allowClickNav: '<?', | ||
| description: '@', | ||
| wizardData: '=', | ||
| onShow: '=?', |
There was a problem hiding this comment.
So it seems like for our callback functions we are using "=" or "<", should we be looking to using real output bindings "&". On the other hand, I think if we do use "&" we have to change our API to use $event obj.
There was a problem hiding this comment.
@dgutride ^^ any thoughts on this? -thanks
There was a problem hiding this comment.
We should probably take a second pass after conversion to look at output bindings - that would really change the interface quite a bit and isn't completely necessary right now. The callback functions won't stop working anytime soon and will allow for easier migration. @dtaylor113
There was a problem hiding this comment.
Ok, sounds good to me. Thanks!
| 'use strict'; | ||
|
|
||
| var firstRun = true, | ||
| ctrl = this; |
There was a problem hiding this comment.
Shouldn't this be in an $onInit?
| }; | ||
|
|
||
| ctrl.steps = []; | ||
| ctrl.context = {}; |
There was a problem hiding this comment.
I'm ok with variable initialization to empty values outside of $oninit if you are - I think if we are doing any complex checks - i.e. see an if() outside of the init, I like to move it, though.
There was a problem hiding this comment.
I'd prefer to see all initialization together rather than scattered between function definitions, etc.
There was a problem hiding this comment.
@jeff-phillips-18, @dtaylor113 - I'll refactor all initialization first thing tomorrow.
| } | ||
| }; | ||
|
|
||
| this.addStep = function (step) { |
There was a problem hiding this comment.
I notice we have defined functions in three different ways. 1. this.funcName = function (..., 2. var foo = function bar(), and 3. ctrl.funcName = function(... What's the diff between 1 and 3? -thanks
There was a problem hiding this comment.
1 and 3 are identical, just a remnant of refactoring to not use scope anymore - I tried to clean those up as I went through but there were quite a few of them. 2 is internal and not exposed via the controller.
There was a problem hiding this comment.
@dtaylor113 - I'll clean this up tomorrow, too - please hold . . .
| }; | ||
|
|
||
| ctrl.steps = []; | ||
| ctrl.context = {}; |
There was a problem hiding this comment.
I'd prefer to see all initialization together rather than scattered between function definitions, etc.
| if (angular.isUndefined(ctrl.allowClickNav)) { | ||
| ctrl.allowClickNav = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Seems like all this should be in $onInit
| if (angular.isUndefined(ctrl.allowClickNav)) { | ||
| ctrl.allowClickNav = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
This should be done in $onInit
| ctrl.title = ctrl.stepTitle; | ||
| ctrl.step.addStep(ctrl); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Prefer to see $onInit as the first thing in the controller function, easier to read/understand.
There was a problem hiding this comment.
Just weird that $onInit is called AFTER $onchange, making charts difficult since they rely on c3 to generate the chart in-the-correct-order :-)
|
LGTM :-) |
This completes the wizard conversion from directive syntax to component syntax. No unit tests had to be changed to support this (all pass, linters pass and test pages work as expected).