Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

test(modal): Add test for controllerAs syntax #2091

Closed
wants to merge 1 commit into from

Conversation

gsklee
Copy link
Contributor

@gsklee gsklee commented Apr 21, 2014

Let's add this since it's kinda the recommended way to do things since v1.1.5.

@bekos
Copy link
Contributor

bekos commented Apr 21, 2014

@gsklee Nice PR and with proper test! LGTM but what about supporting a syntax in the controller attribute: TestCtrl as test instead of adding a new configuration attribute?

@gsklee
Copy link
Contributor Author

gsklee commented Apr 21, 2014

@bekos The available options for $modal.open() looks like an intentional mock on Angular's official Directive Definition Object so I'm just following that as well. As far as I'm aware of, we can't specify a Directive Definition Object's controller property as TestCtrl as test, but a separate controllerAs property is needed, so might as well just follow the official API design here to avoid some potential confusion?

@bekos
Copy link
Contributor

bekos commented Apr 21, 2014

@gsklee My suggestion was to avoid the extra confusion, but I think you are right, forget about it :-)

@pkozlowski-opensource
Copy link
Member

@gsklee thnx for the PR, what you are saying makes sense, but we've got the support for the controller as syntax already. We are using the same syntax as the ng-controller directive, so you can write: controller: 'MyCtrl as myCtrl'.

We are not trying to mimick the directive definition object, but rather a route definition one (modal are a lot like routes if you think about it). While it is true that the route definition object has the controllerAs property I find it really strange, since the $controller service already knows how to handle both a controller specified as string, class and understand the MyCtrl as myCtrl syntax. So I'm not clear why guys at AngularJS decided to have 2 properties...

I don't know, I'm not sooooo keen on adding more bytes for the case that we already do support. WDYT?

@bekos
Copy link
Contributor

bekos commented Apr 21, 2014

@pkozlowski-opensource it was ringing some bells but I couldn't remember exactly what :-) Thanks for clarifying this. Probably we should add this to the docs and merge a similar to @gsklee's, test case.

@gsklee
Copy link
Contributor Author

gsklee commented Apr 21, 2014

@pkozlowski-opensource @bekos Thanks for the clarification, in that case I'll revert the code change but leave the (modified) test case and doc change.

@pkozlowski-opensource
Copy link
Member

@gsklee 👍

@gsklee gsklee changed the title feat(modal): Add controllerAs support test(modal): Add test for controllerAs syntax Apr 21, 2014
@gsklee
Copy link
Contributor Author

gsklee commented Apr 21, 2014

PR updated ;-)

@pkozlowski-opensource
Copy link
Member

Thnx!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants