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

Fix Callback Routing #673

Merged
merged 3 commits into from
Jan 8, 2016
Merged

Conversation

IdanCo
Copy link
Contributor

@IdanCo IdanCo commented Dec 13, 2015

Function 'removed' in TreeNodeController referred to parent scope, but callback was set in the same scope. Deleted redundent function from TreeNodeController, and updated the function in TreeNodesController to trigger callback.

… callback was set in the same scope. Deleted redundent function from TreeNodeController, and updated the function in TreeNodesController to trigger callback
@IdanCo
Copy link
Contributor Author

IdanCo commented Dec 13, 2015

btw, I recommend doing gulp test before merging, I couldn't do it from my office computer...

@@ -456,7 +450,7 @@
// and the 'max-depth' attribute in `ui-tree` or `ui-tree-nodes`.
// the method can be overrided
callbacks.accept = function (sourceNodeScope, destNodesScope, destIndex) {
return !(destNodesScope.nodropEnabled || destNodesScope.outOfDepth(sourceNodeScope));
return !(destNodesScope.nodropEnabled || destNodesScope.$treeScope.nodropEnabled || destNodesScope.outOfDepth(sourceNodeScope));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't mine!
Probably an earlier fix that some how slipped 'gulp build'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you've merged master into this branch, or rebased onto master?

@Aaronm14
Copy link

There's a little conversation going on about this in the issues here: #586

Thanks for fixing, hopefully it will get merged here soon.

@Voles
Copy link
Member

Voles commented Dec 20, 2015

@IdanCo thanks for your work! I've reviewed your PR and it's looks ok to merge. Do you have some time to bring your branch up to date with master?

I suggest to remove the /dist files from your PR, as these will be generated when we release a new version.

Thanks!

@Voles Voles added the reviewed label Dec 20, 2015
@Voles
Copy link
Member

Voles commented Jan 6, 2016

@IdanCo any updates on this? :-)

@IdanCo
Copy link
Contributor Author

IdanCo commented Jan 6, 2016

On it!

@IdanCo
Copy link
Contributor Author

IdanCo commented Jan 6, 2016

Done!

Voles pushed a commit that referenced this pull request Jan 8, 2016
@Voles Voles merged commit ef449bb into angular-ui-tree:master Jan 8, 2016
@Voles
Copy link
Member

Voles commented Jan 8, 2016

Thank you very much, @IdanCo! 🍰

@willbastian
Copy link

Any thought on the issue documented with the removed callback being hit when re-ordering nodes?

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.

None yet

4 participants