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

Event handlers in links inside toasts? #14

Closed
drothlis opened this Issue Dec 19, 2014 · 15 comments

Comments

Projects
None yet
6 participants
@drothlis

drothlis commented Dec 19, 2014

Hi there

I wonder if it's possible to have an <a ng-click=...> in the toast
content. For example: http://plnkr.co/edit/PmBFNugm9s0TlVLbpmx2?p=preview

It doesn't seem to work -- clicking on the link doesn't seem to call the
function as I'd expect. I've tried marking the content value with
$sce.trustAsHtml but it doesn't make any difference.

Any ideas?

Thanks for your time (and for ngToast!).

Dave.

@rictorres

This comment has been minimized.

Show comment
Hide comment
@rictorres

rictorres Dec 19, 2014

Contributor

+1
i'd love to see this as well

Contributor

rictorres commented Dec 19, 2014

+1
i'd love to see this as well

@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Dec 19, 2014

Owner

I think with current implementation there is no way to compile any directives on the toast message content. (But <a onclick="nativeMethod()"> would work of course.)

I am going to work on that when I find chance, and there will be a new release.

Owner

tameraydin commented Dec 19, 2014

I think with current implementation there is no way to compile any directives on the toast message content. (But <a onclick="nativeMethod()"> would work of course.)

I am going to work on that when I find chance, and there will be a new release.

tameraydin added a commit that referenced this issue Dec 19, 2014

Merge pull request #15 from tameraydin/feature/content-compiling
Implemented post-compilation option. Closes #14
@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Dec 20, 2014

Owner

I introduced an extra option called compileContent which will allow you to reach container scope by re-compiling transcluded content in case of enabling, with the version 1.3.0. It comes with default false value and needs to be overridden per each toast message.

I implemented it in this way, because the developer should be aware of possible vulnerability issues and enable it with his own responsibility.

$sce.trustAsHtml is still needed, so the final code would look like:

angular
  .module('myApp', ['ngToast'])
  .controller('myCtrl', ['$scope', '$sce', 'ngToast', function($scope, $sce, ngToast) {
    $scope.label = 'Click me';
    $scope.log = function() {
      console.log('Clicked!')
    };
    ngToast.create({
      content: $sce.trustAsHtml('<a ng-click="log()">{{label}}</a>'),
      compileContent: true // or any $scope object (truthy values will take the parent scope as default)
    });
  }]);
Owner

tameraydin commented Dec 20, 2014

I introduced an extra option called compileContent which will allow you to reach container scope by re-compiling transcluded content in case of enabling, with the version 1.3.0. It comes with default false value and needs to be overridden per each toast message.

I implemented it in this way, because the developer should be aware of possible vulnerability issues and enable it with his own responsibility.

$sce.trustAsHtml is still needed, so the final code would look like:

angular
  .module('myApp', ['ngToast'])
  .controller('myCtrl', ['$scope', '$sce', 'ngToast', function($scope, $sce, ngToast) {
    $scope.label = 'Click me';
    $scope.log = function() {
      console.log('Clicked!')
    };
    ngToast.create({
      content: $sce.trustAsHtml('<a ng-click="log()">{{label}}</a>'),
      compileContent: true // or any $scope object (truthy values will take the parent scope as default)
    });
  }]);
@drothlis

This comment has been minimized.

Show comment
Hide comment
@drothlis

drothlis Dec 20, 2014

Thanks for the quick fix & release! I confirm that it works. Just FYI the 1.3.0 release still says "1.2.1" at the top of the js file.

This works well when the <toast> directive is inside the scope of the controller creating the notification. What do you think is the best practice when you have several controllers that all want to create toasts with actions/links in them (where the action is a function on the controller's scope), and the <toast> directive isn't inside the scope of all of the controllers? From what I've read of the Angular docs, I gather that you'd create a toast with something like this:

ngToast.create({
    content: $sce.trustAsHtml('<a ng-click="$root.broadcast(\'my.event\')">Click me</a>'),
    compileContent: true
});

...and have the controller listen for my.event. Is there a better or more idiomatic way?

drothlis commented Dec 20, 2014

Thanks for the quick fix & release! I confirm that it works. Just FYI the 1.3.0 release still says "1.2.1" at the top of the js file.

This works well when the <toast> directive is inside the scope of the controller creating the notification. What do you think is the best practice when you have several controllers that all want to create toasts with actions/links in them (where the action is a function on the controller's scope), and the <toast> directive isn't inside the scope of all of the controllers? From what I've read of the Angular docs, I gather that you'd create a toast with something like this:

ngToast.create({
    content: $sce.trustAsHtml('<a ng-click="$root.broadcast(\'my.event\')">Click me</a>'),
    compileContent: true
});

...and have the controller listen for my.event. Is there a better or more idiomatic way?

@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Dec 20, 2014

Owner

I think in your case, your approach seems the most appropriate, since you have to deal with the communication of several controllers, and the broadcasting fit for that purpose. But as a small notice, I would rather use a scope method instead of broadcasting the event right on the element, which may give you more cheaper/easier ways to communicate such as using $emit within the scope instead of rootScope, or using require on directives (just as 'docsTabsExample' here).

... and thank you for informing about the version, now they are up-to-date.

Owner

tameraydin commented Dec 20, 2014

I think in your case, your approach seems the most appropriate, since you have to deal with the communication of several controllers, and the broadcasting fit for that purpose. But as a small notice, I would rather use a scope method instead of broadcasting the event right on the element, which may give you more cheaper/easier ways to communicate such as using $emit within the scope instead of rootScope, or using require on directives (just as 'docsTabsExample' here).

... and thank you for informing about the version, now they are up-to-date.

@drothlis

This comment has been minimized.

Show comment
Hide comment
@drothlis

drothlis Dec 20, 2014

Thanks for that link to the docsTabsExample. I didn't know about require.

However I don't think that will help me in this case, because I wouldn't want to modify the ngToast source code to require my controller (and I'm not sure if it would find controllers that are siblings, not parents, in the dom).

To be clear, I don't need to communicate between controllers. I just want to use ngToast to display notifications that can call an action back on the same controller that created the notification.

I had an idea: What if ngToast's compileContent could optionally take a scope instead of true, and then when you call $compile, pass in that scope. See it working here -- note that <toast> is a sibling, not a child, of Ctrl.

What do you think of this approach? If you like it I can submit a pull request. It seems fairly idiomatic javascript to me (essentially I'm passing in a closure), but I have no idea if it's idiomatic Angular. Also, I have no idea what will happen if the controller is destroyed while the toast message is still visible.

drothlis commented Dec 20, 2014

Thanks for that link to the docsTabsExample. I didn't know about require.

However I don't think that will help me in this case, because I wouldn't want to modify the ngToast source code to require my controller (and I'm not sure if it would find controllers that are siblings, not parents, in the dom).

To be clear, I don't need to communicate between controllers. I just want to use ngToast to display notifications that can call an action back on the same controller that created the notification.

I had an idea: What if ngToast's compileContent could optionally take a scope instead of true, and then when you call $compile, pass in that scope. See it working here -- note that <toast> is a sibling, not a child, of Ctrl.

What do you think of this approach? If you like it I can submit a pull request. It seems fairly idiomatic javascript to me (essentially I'm passing in a closure), but I have no idea if it's idiomatic Angular. Also, I have no idea what will happen if the controller is destroyed while the toast message is still visible.

@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Dec 20, 2014

Owner

There are some points that I didn't get; you said that you need to modify ngToast source but what I have mentioned was requiring an upper controller at the controller that you are calling ngToast.create, which doesn't require any modification. And other thing was "to display notifications that can call an action back on the same controller that created the notification"; actually this is exactly what is implemented now. But maybe I'm missing some point here or didn't fully understand the problem, anyway.

Your idea is not much different than what is implemented now, even it will be kind of more generic in terms of compiling the content at the parent scope (the scope that ngToast is created) all the time. So technically it is OK, but I'm not sure if we are doing it idiomatically right. And that's why, if it is possible, can you create a minimal demonstration of your problem on a Plunkr, so I can also see the overall situation and think for any approaches? Also it is helpful for me to see what kind of use cases that ngToast can be used in. Thanks..

Owner

tameraydin commented Dec 20, 2014

There are some points that I didn't get; you said that you need to modify ngToast source but what I have mentioned was requiring an upper controller at the controller that you are calling ngToast.create, which doesn't require any modification. And other thing was "to display notifications that can call an action back on the same controller that created the notification"; actually this is exactly what is implemented now. But maybe I'm missing some point here or didn't fully understand the problem, anyway.

Your idea is not much different than what is implemented now, even it will be kind of more generic in terms of compiling the content at the parent scope (the scope that ngToast is created) all the time. So technically it is OK, but I'm not sure if we are doing it idiomatically right. And that's why, if it is possible, can you create a minimal demonstration of your problem on a Plunkr, so I can also see the overall situation and think for any approaches? Also it is helpful for me to see what kind of use cases that ngToast can be used in. Thanks..

@drothlis

This comment has been minimized.

Show comment
Hide comment
@drothlis

drothlis Dec 20, 2014

Sorry, I did a terrible job of explaining that (and I obviously didn't fully understand what you meant about require). :-)

Here's a demonstration on plunkr: http://plnkr.co/edit/XkhHPQNGu4INwHpzWqz1?p=preview

My use case is that I have a lot of different controllers on the page, and they all want to create notifications via ngToast. Obviously I can't put the <toast> directive inside all of them at once. And I don't want to move the callback functions (f1 and f2 in my plunkr example) outside of Ctrl1 and Ctrl2 into a common parent controller that the toast directive can access.

Don't spend too much time on this -- with ngToast 1.3.0 I can solve my problem by broadcasting events rather than calling functions. I really appreciate your quick work on implementing compileContent.

drothlis commented Dec 20, 2014

Sorry, I did a terrible job of explaining that (and I obviously didn't fully understand what you meant about require). :-)

Here's a demonstration on plunkr: http://plnkr.co/edit/XkhHPQNGu4INwHpzWqz1?p=preview

My use case is that I have a lot of different controllers on the page, and they all want to create notifications via ngToast. Obviously I can't put the <toast> directive inside all of them at once. And I don't want to move the callback functions (f1 and f2 in my plunkr example) outside of Ctrl1 and Ctrl2 into a common parent controller that the toast directive can access.

Don't spend too much time on this -- with ngToast 1.3.0 I can solve my problem by broadcasting events rather than calling functions. I really appreciate your quick work on implementing compileContent.

@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Dec 20, 2014

Owner

I see the point clearly now. Let's make it work by passing $scope objects then, it will be more handy for such cases. And as you mentioned, it might be problematic when the referenced scope is destroyed but let's leave it to developer's consideration since it is an edge-usage option.

Any PR is welcome :) Thanks for the idea & sharing.

Owner

tameraydin commented Dec 20, 2014

I see the point clearly now. Let's make it work by passing $scope objects then, it will be more handy for such cases. And as you mentioned, it might be problematic when the referenced scope is destroyed but let's leave it to developer's consideration since it is an edge-usage option.

Any PR is welcome :) Thanks for the idea & sharing.

@levithomason

This comment has been minimized.

Show comment
Hide comment
@levithomason

levithomason Dec 21, 2014

Collaborator

@drothlis I see you don't want to move the callbacks to a common parent controller. However, it sounds like a service might be well suited for the functionality you are looking for.

Can you elaborate a bit more on what the callbacks are for and are doing?

Collaborator

levithomason commented Dec 21, 2014

@drothlis I see you don't want to move the callbacks to a common parent controller. However, it sounds like a service might be well suited for the functionality you are looking for.

Can you elaborate a bit more on what the callbacks are for and are doing?

@drothlis

This comment has been minimized.

Show comment
Hide comment
@drothlis

drothlis Dec 21, 2014

@levithomason can you expand on how a service would be used -- do you mean a service that contains the f1 and f2 callbacks? How would ngToast reference the service?

I'm writing the UI for an appliance. I have:

  • a controller or service (not sure which is best yet) that polls the appliance every minute or so and pops up a notification that says "click here to install software" -- the ng-click callback does a POST and redirects to a view that displays a progress bar while the appliance reboots.
  • several controllers to tell the appliance what to do. These display ngToast notifications -- so far these toasts don't have any callbacks associated, but I want to keep my options open and not couple my notification system to my SoftwareUpdateController.

@tameraydin I'll put together a pull request in the next few days.

drothlis commented Dec 21, 2014

@levithomason can you expand on how a service would be used -- do you mean a service that contains the f1 and f2 callbacks? How would ngToast reference the service?

I'm writing the UI for an appliance. I have:

  • a controller or service (not sure which is best yet) that polls the appliance every minute or so and pops up a notification that says "click here to install software" -- the ng-click callback does a POST and redirects to a view that displays a progress bar while the appliance reboots.
  • several controllers to tell the appliance what to do. These display ngToast notifications -- so far these toasts don't have any callbacks associated, but I want to keep my options open and not couple my notification system to my SoftwareUpdateController.

@tameraydin I'll put together a pull request in the next few days.

@levithomason

This comment has been minimized.

Show comment
Hide comment
@levithomason

levithomason Dec 30, 2014

Collaborator

Yep, a service that contains the callbacks. In the end, the callbacks will need to end up on some scope that is availble in some template via some controller.

One ideal way I've seen this done for system wide logic, like toasts and authentication, is to put your callbacks and other data handling logic into a service and attach that service to the $rootScope. Since all templates have access to $rootScope, that service will be available in all templates along with its methods.

I updated your plunker to use a service and I believe it accomplishes what you are after. Let me know if this is the case:

http://plnkr.co/edit/mIEYmqWC9VRGZK4Iid8g?p=preview

Sorry it took me so long to respond, happy holidays!

Edit: Strikeout mistake.

Collaborator

levithomason commented Dec 30, 2014

Yep, a service that contains the callbacks. In the end, the callbacks will need to end up on some scope that is availble in some template via some controller.

One ideal way I've seen this done for system wide logic, like toasts and authentication, is to put your callbacks and other data handling logic into a service and attach that service to the $rootScope. Since all templates have access to $rootScope, that service will be available in all templates along with its methods.

I updated your plunker to use a service and I believe it accomplishes what you are after. Let me know if this is the case:

http://plnkr.co/edit/mIEYmqWC9VRGZK4Iid8g?p=preview

Sorry it took me so long to respond, happy holidays!

Edit: Strikeout mistake.

@NPToita

This comment has been minimized.

Show comment
Hide comment
@NPToita

NPToita Apr 24, 2015

to accommodate drothlis 's requirement (which is also what i'm needing), I made for the following changes to ngToast itself:

  1. in the provider - added the scopeToCompile property as part of the defaults object.
  2. in the toast directive's compileContent check in the link function - add a var scopeToCompile = scope.message.scopeToCompile || scope.$parent and replace "scope.$parent" with scopeToCompile in the $compile() call.

then usage is as followed:
var app = angular.module('myApp', ['ngToast']);
app.controller('myCtrl',['$scope','$sce', 'ngToast', function($scope, $sce, ngToast) {
$scope.toast = function(){
ngToast.create({content : $sce.trustAsHtml('a toast message2'),
compileContent: true,
compileToScope: $scope,
dismissOnClick: false,
dismissButton: true
});
};

$scope.toast2 = function(){
ngToast.create({content: 'linked Toast'})
}
}]);

NPToita commented Apr 24, 2015

to accommodate drothlis 's requirement (which is also what i'm needing), I made for the following changes to ngToast itself:

  1. in the provider - added the scopeToCompile property as part of the defaults object.
  2. in the toast directive's compileContent check in the link function - add a var scopeToCompile = scope.message.scopeToCompile || scope.$parent and replace "scope.$parent" with scopeToCompile in the $compile() call.

then usage is as followed:
var app = angular.module('myApp', ['ngToast']);
app.controller('myCtrl',['$scope','$sce', 'ngToast', function($scope, $sce, ngToast) {
$scope.toast = function(){
ngToast.create({content : $sce.trustAsHtml('a toast message2'),
compileContent: true,
compileToScope: $scope,
dismissOnClick: false,
dismissButton: true
});
};

$scope.toast2 = function(){
ngToast.create({content: 'linked Toast'})
}
}]);

@tameraydin

This comment has been minimized.

Show comment
Hide comment
@tameraydin

tameraydin Apr 28, 2015

Owner

now with 1.5.1, compileContent can also take $scope object to bind. (as backward compatibility boolean values are still supported: true will use parent scope)

Owner

tameraydin commented Apr 28, 2015

now with 1.5.1, compileContent can also take $scope object to bind. (as backward compatibility boolean values are still supported: true will use parent scope)

@taharedape

This comment has been minimized.

Show comment
Hide comment
@taharedape

taharedape Apr 10, 2017

Thanks so much @levithomason you saved me hours! i was wondering which the ng-click does not work within the same controller and only with factory function :/

taharedape commented Apr 10, 2017

Thanks so much @levithomason you saved me hours! i was wondering which the ng-click does not work within the same controller and only with factory function :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment