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

$scope.$on('destroy') should be '$destroy' #908

Merged
merged 1 commit into from
Apr 5, 2017
Merged

$scope.$on('destroy') should be '$destroy' #908

merged 1 commit into from
Apr 5, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Apr 4, 2017

$scope.$on('destroy', ...) listens for a custom event called destroy, which we never produce.

The intent behind that code was definitely to listen for the $destroy event which happens when destroying the scope.

'destroy' would need to be a custom event
the intent behind that code was definitely the $destroy event which happens when destroyin the scope
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2017

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/ef2eb3034c5635eae095c8850a153bb1e0fbd94e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@mzazrivec mzazrivec added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017
@mzazrivec mzazrivec merged commit eb42b56 into ManageIQ:master Apr 5, 2017
@himdel himdel deleted the onDestroy branch April 5, 2017 10:16
simaishi pushed a commit that referenced this pull request Apr 5, 2017
$scope.$on('destroy') should be '$destroy'
(cherry picked from commit eb42b56)
@simaishi
Copy link
Contributor

simaishi commented Apr 5, 2017

Fine backport details:

$ git log -1
commit b78f07c58418409abd860e113245ba532902ebd3
Author: Milan Zázrivec <[email protected]>
Date:   Wed Apr 5 12:02:13 2017 +0200

    Merge pull request #908 from himdel/onDestroy
    
    $scope.$on('destroy') should be '$destroy'
    (cherry picked from commit eb42b56189ac994b5114865b0c19284f8160a245)

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

Successfully merging this pull request may close these issues.

4 participants