Skip to content
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

Added support for 'controllerAs' syntax in event callbacks #235

Closed
wants to merge 4 commits into from
Closed

Added support for 'controllerAs' syntax in event callbacks #235

wants to merge 4 commits into from

Conversation

niclasgillberg
Copy link

Added support for using controllerAs syntax when defining event callbacks such as onDrop.

Instead of always setting the scope as context to the callback, it now sets the constructor to the context is the constructor exists on the scope. Uses of this in the callback should therefore refer to the controller.

@codef0rmer
Copy link
Owner

@niclassahlin: thanks for the PR. However, I'd already implemented support for controllerAs syntax. Please take a look at this example: https://github.com/codef0rmer/angular-dragdrop/blob/master/demo/dnd-ctrl-as-syntax.html

Did you find any issue/bug in existing implementation?

@niclasgillberg
Copy link
Author

@codef0rmer The syntax worked so it referred to the correct function after the parsing. However, the function is always executed in the context of the scope, which means if you use a prototype based controller then this will not be what is expected.

You can see an example of this, where I use a Typescript class to generate the controller function (you'll see the exception in the console).

This is basically how we have built all of our Angular components. We try to use the controller object and avoiding the scope as much as possible to get a smoother transition to Angular 2.

Sorry if the title is a bit misleading.

@niclasgillberg
Copy link
Author

@codef0rmer Do you have any input? We are quite dependent on this fix and I don't want to diverge from the standard library too much, so any feedback would be highly appreciated.

@codef0rmer
Copy link
Owner

Hey @niclassahlin, I'm slightly loaded, I'll take some time out this weekend to review your PR.

@codef0rmer
Copy link
Owner

@niclassahlin I believe only following change is required. If you log this to the console in the dropCallback, it should print an object (consisting properties/methods bound on this in the controller) if ctrlAs syntax is used, otherwise it should print a scope object of the draggable item.

Can you please confirm if following fix works for you?

1f6b7f2

@codef0rmer codef0rmer added the bug label Dec 26, 2015
@niclasgillberg
Copy link
Author

@codef0rmer I added your fix instead of mine and it seems to work. I think the difference between our fixes are how we choose the context when there is a method on both the scope and the constructor, but I think that it is uncommon to mix the two styles so I don't think it is that important.

So yes, your fix seems to fix it :)

Thanks!

@codef0rmer
Copy link
Owner

@niclassahlin: Thanks. Regarding two styles, you can define a method on $scope as well as this but can not used in the template if it were defined on $scope. So that situation will never occur?

@niclasgillberg
Copy link
Author

@codef0rmer theoretically it could occur, but I don't see any practical reason why it would. I think that either $scope or this should be used and not a mix of them. In addition, since $scope is the default context then the behavior is the same as it was before, so I don't think that it is an issue.

@codef0rmer
Copy link
Owner

@niclassahlin Alright my friend, I'll merge my commit 👍

@niclasgillberg
Copy link
Author

Sweet 👍

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

Successfully merging this pull request may close these issues.

2 participants