search

m-e-conroy / angular-dialog-service

A complete AngularJS service with controllers and templates for generating application modals and dialogs for use with Angular-UI-Bootstrap and Twitter Bootstrap
MIT License
618 stars 228 forks source link

Remove $scope.$destory from close method. Fixes #120 #121

Closed Luftzig closed 8 years ago

Luftzig commented 9 years ago

ui-bootstrap 0.13.0 handles destroying the scope itself. Haven't tested with 0.12.1, though.

dougmoscrop commented 9 years ago

Nitpick: You have a typo in your commit message / PR description (destory vs destroy) - can make grepping history difficult.

Are you sure this is the right solution? I figured the $destroy is in there because of DOM manipulation - so would this not introduce a leak?

A recent commit (https://github.com/m-e-conroy/angular-dialog-service/commit/e122efebe9bf06c6703e3452ddb219e8da86fa63) added an option for animation. @m-e-conroy can comment further, but should this maybe use that flag to determine if it should call $scope.$destroy (if animations are off) or use the $animate service to wait for 'leave' and then call the destroy? (see http://stackoverflow.com/questions/18937758/how-can-i-defer-the-scope-element-destroy-event-so-i-can-animate-the-elemen)

m-e-conroy commented 9 years ago

I'm not sure how I feel about this, I'm not seeing it cause an error or anything if the $scope is already destroyed. My inclination is to leave it for now, till I get more time to go through it - currently on a tight deadline this summer.

niemyjski commented 8 years ago

Can you please reopen this if it's still an issue with the latest version.