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

Removed outOfBounds variable #856

Merged
merged 7 commits into from
Dec 20, 2016
Merged

Conversation

herumtreiber
Copy link
Contributor

Hey,

I found several bugs that are caused by the calculation of outOfBounds not working correctly. The PR #739 was committed to remove the faulty implementation and resolve at least #622 and #763 .

The pull request was unfortunately never merged since it contained merge conflicts. I resolved the conflicts since I really need this fix for our project. I did not test all possible impacts the removing of the variable might have, though.

best,
Sven

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 36.624% when pulling 2db0063 on herumtreiber:issue-622 into 17b66fb on angular-ui-tree:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 36.624% when pulling a5e9e4a on herumtreiber:issue-622 into 17b66fb on angular-ui-tree:master.

@Voles
Copy link
Member

Voles commented Oct 31, 2016

@herumtreiber thanks for your work! Can you remove the files in /dist from your PR?
What impact does the removal of the outOfBounds calculation have on the behaviour of the component?

This reverts commit a5e9e4a.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 36.624% when pulling 7872c5d on herumtreiber:issue-622 into 17b66fb on angular-ui-tree:master.

@herumtreiber
Copy link
Contributor Author

@Voles no problem. I reverted my commit with the dist files.

We are using a version with the change in our project at the moment and I couldn't see any negative impacts on the component, yet. I guess that the outOfBounds variable was introduced for performance reasons, but I didn't experience a negative effect on the overall performance either.

We are using the component for drag and drop between multiple trees and also to create hierarchies.

@herumtreiber
Copy link
Contributor Author

Hey @Voles ,

any news about the status of this pull request?

@Voles Voles merged commit 390d9a9 into angular-ui-tree:master Dec 20, 2016
@Voles
Copy link
Member

Voles commented Dec 20, 2016

@herumtreiber thanks very much for your time and effort! I'll create a new release soon.

@herumtreiber
Copy link
Contributor Author

Thank you! :)

@Voles
Copy link
Member

Voles commented Dec 21, 2016

@herumtreiber see https://github.com/angular-ui-tree/angular-ui-tree/releases/tag/v2.22.2 🌻

@herumtreiber
Copy link
Contributor Author

Woohoo! That was quick! Thanks a lot! :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants