dwmkerr / angular-modal-service

Modal service for AngularJS - supports creating popups and modals via a service.

MIT License

626 stars 321 forks source link

Invoke optional controller $onInit function #257

Closed DougKeller closed 5 years ago

DougKeller commented 5 years ago

https://docs.angularjs.org/guide/component#component-based-application-architecture

1.6.x introduced many mechanisms to help AngularJS 1.X projects begin conforming with Angular 2+'s component-based structure. Referencing the link above:

Components have a well-defined lifecycle: Each component can implement "lifecycle hooks". These are methods that will be called at certain points in the life of the component.

One of the most useful hooks defined is the $onInit hook, which is run after the element has been constructed. An additional perk of this hook is that it can be tested individually without having to mock out any setup the controller has to do once it has been initialized.

Because AngularModalService manually initializes the modal's controller, this lifecycle hook is never invoked. This pull request checks that the initialized controller defines an $onInit function, and if it does, calls it before resolving the modal.

codecov[bot] commented 5 years ago

Codecov Report

Merging #257 into master will increase coverage by 0.27%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   85.04%   85.32%   +0.27%     
==========================================
  Files           1        1              
  Lines         107      109       +2     
==========================================
+ Hits           91       93       +2     
  Misses         16       16

Impacted Files	Coverage Δ
src/angular-modal-service.js	`85.32% <100%> (+0.27%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 22cda41...1d6bc14. Read the comment docs.

dwmkerr commented 5 years ago

Awesome! One minor comment, otherwise looks great! Thanks for the tests too.

DougKeller commented 5 years ago

@dwmkerr Added that comment. Let me know if there's anything else - otherwise thanks for reviewing this quickly!

Also, this PR addresses #173 so that can be resolved once this is merged. I didn't realize this issue already had a similar PR (#190) before submitting it.

dwmkerr commented 5 years ago

Thanks so much for this @DougKeller! I've just released now, version 0.15.1. Sorry for the delay, busy week! I'll check the other PR and Issue you mentioned and close them too. Cheers!