ezraroi / ngJsTree

Angular Directive for the famous JS Tree
http://ezraroi.github.io/ngJsTree/
MIT License
270 stars 101 forks source link

Several fixes to better work with bindings #77

Closed SuricateCan closed 8 years ago

SuricateCan commented 8 years ago

Hi, I've talked about some of these fixes on #75.

Here is the list of accomplishments on this commit:

NOTE: These changes CAN break binding calls if they expect the li#node as this.


This change is Reviewable

SuricateCan commented 8 years ago

After struggling with the whole karma jasmine thing, I manage to get all code to run correctly (even corrected a bug because of the failed tests) and all tests to pass.

ezraroi commented 8 years ago

Hi, first thanks. Few points:

  1. I don't think using $$phase is a good practice as this is internal to angular
  2. Can you explain the jqeury plugin?
SuricateCan commented 8 years ago

Hi @ezraroi, let me explain:

  1. There is no problem in using this flag to just CHECK the current phase. I do not touch it to set, only to get. The problem is that, whenever a digest is in place, if you call $apply, an exception is thrown (for good reason). This directive is wrapping a third party code that is foreign to angular, so we need to to call apply if not digesting already, as explained on the docs (https://docs.angularjs.org/api/ng/type/$rootScope.Scope#$apply). For the use of $$phase, a quick search on google and you'll see that there is no problem in reading that.
  2. The jquery plugin is making a simple task. It overrides all trigger functions internal to jstree (this.trigger, dnd._trigger and context._trigger) and calls the function set on "alltrigger" option, saying that a specific event occurred. Why you need that? To make all events trigger a digest on angular. Again, events inside jstree are foreign to angular, and we need these changes to propagate to our angular environment. Also, I've included an array variable to check if the current trigger is already handled by the user. If so, there is no need for a digest call, because the call to the user handler is already on a digest cycle.

Another way to trigger a digest for angular is wrapping the call with $timeout, but I don't think that its a good use in this scenario.

SuricateCan commented 8 years ago

Another thing I feel like explaining is the use of $timeout wrapping manageEvents and wrapping onAdded contents:

On manageEvents, if the function is called immediately with the rest of the code, jstree triggers , specially those that trigger on document, will be called after the user handling code. One bad example of it is if you try to bind to dnd_stop.vakata. Without the $timeout, your code on scope will execute before the element is actually moved, causing all node information to be "incorrect".

On onAdded, I added the $timeout so javascript wont hang. I don't know exactly why it would hang on while(blocked) (I don't see any reason for this block to exist in the first place, but I wouldn't change the basics of your code).

ezraroi commented 8 years ago

It was design decision not trigger digest on every jstree event, it might have been wrong. In my projects, I just ran the event handler in $apply or $timeout when I needed.

ezraroi commented 8 years ago

About the while loop.. I don't really remember now as it was a long time ago, but it was needed on this I am pretty sure

SuricateCan commented 8 years ago

Maybe the while loop was to prevent event handlers to call the onAdded function again? In any case, now that it is inside a $timeout, I don't think it is necessary, but again, it's your call.

BTW, I am glad I could contribute to your project.