Skip to content

Allow element and attr injections #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2013

Conversation

lmessinger
Copy link
Contributor

The controller in ui-router is really a 'directive controller' as it is tied with $ViewDirective. In Angular.js this type of controller is injectable with $element and $attr, so i've copied the same to here as well.

I needed it to manipulate the $element (specifically, for animation of the ui-view).

This is a 'directive controller' therefore needed those injectables
@jeme
Copy link
Contributor

jeme commented Apr 2, 2013

Actually I think that is wrong.

This suddenly allow for DOM manipulation from the View Controllers, and IMO that should be discouraged. These controllers are meant to handle what data is brought into the view.

This is also in line with the original Angular routing implementation, if you take a look at their "ng-view", you can't access the attr nor the element from the controllers.

Animations should be done by options, paired directives or something else, but not by the viewController...

I think the easy way to see when a controller is a directive controller is that it would have a one-to-one relation with it's implementation because they are bound together, here you will have thousands of different implementations of a controller.

That is my opinion anyway.

@ksperling
Copy link
Contributor

@lmessinger where in core AngularJS is this supported? I don't thing ng-view does this.

@lmessinger
Copy link
Contributor Author

Directives in general do support element/attr injection - see here: http://docs.angularjs.org/guide/directive. But that's right - ngView does not support it, it loses it. Since routers are about transitions, and transitions are about animations, I thought it's important to allow this flexibility.

nfx added a commit that referenced this pull request Apr 3, 2013
Allow element and attr injections
@nfx nfx merged commit 63df6c7 into angular-ui:master Apr 3, 2013
@jeme
Copy link
Contributor

jeme commented Apr 3, 2013

@lmessinger Directive controllers does yes, but in my mind your mixing things together here... A directive controller is all knowledgeable about a specific directive, like in a normal Java, C, or .NET app you could imagine having controllers for components like Buttons, TextBoxes, and bigger components like Grids, Calendars etc... Which can manage low level things like "disabled", "border styles" and much more"... but they aren't the wizer about your business model as these are all general purpose, reusable components...

That is what directives are to me...

A View controller should not care about how things are presented, and therefore not about how the changes of a page is rendered, it should only concern it self about the data it needs to provide to the view (through the scope in this case)... And business level operations... Save Record, Discard, Etc...

And I even think that allowing these controllers to manipulate the DOM is directly against the spirit of Angular. It kind of defeats the entire purpose of all those directives as is...

Here is a little snippet from http://docs.angularjs.org/guide/dev_guide.mvc.understanding_controller

Do not use controllers for:

  • Any kind of DOM manipulation — Controllers should contain only business logic. DOM manipulation—the presentation logic of an application—is well known for being hard to test. Putting any presentation logic into controllers significantly affects testability of the business logic. Angular offers databinding for automatic DOM manipulation. If you have to perform your own manual DOM manipulation, encapsulate the presentation logic in directives.
  • Input formatting — Use angular form controls instead.
  • Output filtering — Use angular filters instead.
  • To run stateless or stateful code shared across controllers — Use angular services instead.
  • To instantiate or manage the life-cycle of other components (for example, to create service instances).

Take note of the very first bullet

@lmessinger
Copy link
Contributor Author

@jeme thanks - I understand but i beg to differ. Angular rightfully decouples a standard controller from the presentation , but here the View directive is as its name reveals... the presentation. In other words, when users define a ui-view template html, users define the physical layout, the presentation. That's why they might need a way to control that presentation. indeed, in my project's design i use the ui-view solely to define the layout presentation. no data, no {{}} at all.

I think that's the reason why Angular team allowed injection of an element to a directive since the directive is the component that deals with the presentation side.

On any case I think this is about different interpretations of the 'spirit of Angular' (a new goddess? :) ) so opinions might differ. I suggest to leave it as an option for those who might need it. thanks.

@jeme
Copy link
Contributor

jeme commented Apr 3, 2013

@lmessinger They indeed need a way to control the presentation in templates, that is why you can use Angular directives inside templates. But it's not the job of the controller. Keep in mind that you can very well end up with an application only consisting of these view controllers and services...

If you need to do some animation or something on the entire view that should either be incorporated into the ui-view directive or you could write an entirely new directive for it, alternatively if you wanted to make it extendable an animate provider could be created to add support for this.

After all... where are your "standard controllers" anyways if this is not the ones?

I don't think it's a fluke they left it out of the Original ng-view... I actually think it was on purpose. After all they mention "ng-view" specifically in the series of Angular MVC

http://docs.angularjs.org/guide/dev_guide.mvc.understanding_view

The view has knowledge of the controller through Angular directives, such as ngController and ngView, and through bindings of this form: {{someControllerFunction()}}. In these ways, the view can call functions in an associated controller function.

@ksperling
Copy link
Contributor

I agree with @jeme on this. If you think $element/$attr injection is important, I think that's a case that should be made and a discussion to be had for AngularJS in general, i.e. ng-view, not something we add onto ui-view.

As far as animation is concerned, there's already #22. I don't know what shape exactly the API is going to take, but in either case for animations you'd want to be able to access the DOM element corresponding to the previous and new content being placed into the ui-view, not just the new one.

The patch also changes of the name of the data attribute for the controller; again this is a deviation from ng-view that hasn't been justified in the discussion or comments (as far as I'm aware the Angular convention is to name these after the directive itself; I don't think it makes any sense to use the individual view instance name for it).

@ProLoser
Copy link
Member

ProLoser commented Apr 9, 2013

@nfx I appreciate your intentions however this issue had not reached a consensus when you merged it. You must review these issues, (especially those with opposition) and at the very least state your reasons before simply merging them in.

@nfx
Copy link
Contributor

nfx commented Apr 9, 2013

@ProLoser sure

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.

5 participants