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

Prevents child node scope with no children to be counted in depth. #388

Merged
merged 4 commits into from
May 8, 2015

Conversation

Whyves
Copy link
Contributor

@Whyves Whyves commented Feb 23, 2015

When specifying maxDepth and using a template in ng-include to generate the tree nodes, a template is created for empty children. This causes the logic in maxSubDepth to include the depth of non-existent children thus going over the max depth limit and preventing node moves even when they were valid. I have added a test for this.

@Voles
Copy link
Member

Voles commented May 2, 2015

@Whyves nice improvement! Can you bring your PR up-to-date with the master branch?

@Voles Voles added the reviewed label May 2, 2015
@Whyves
Copy link
Contributor Author

Whyves commented May 3, 2015

Tried to rebase but you changed the name attribute in package.json to "AngularJS UI Tree" and that prevents me from running "npm install" it complains about "AngularJS UI Tree" to be invalid. Putting "-" instead of spaces fix the problem. May I suggest that you rename "AngularJS UI Tree" to "angular-ui-tree" which seems more standard. I am under Ubuntu. Once this is fixed, I will rebase my changes.

@Voles
Copy link
Member

Voles commented May 3, 2015

Fixed, sorry for the inconvenience...

@Whyves
Copy link
Contributor Author

Whyves commented May 4, 2015

There you go, PR up-to-date. Sorry for the multiple commits. I had a few issues with jscs :-)

Voles pushed a commit that referenced this pull request May 8, 2015
Prevents child node scope with no children to be counted in depth.
@Voles Voles merged commit 88aeb84 into angular-ui-tree:master May 8, 2015
@Voles
Copy link
Member

Voles commented May 8, 2015

Thanks for this, @Whyves!

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

2 participants