Zicrael / ngx-tree-dnd

Angular 7 support, data sortable draggable smart tree.
https://ngx-tree-dnd.stackblitz.io
MIT License
39 stars 11 forks source link

Added fixes to reduce change detection cycles #28

Closed njofce closed 5 years ago

njofce commented 5 years ago

1) Runs drag listeners outside of Angular zone to reduce change detection cycles, and triggers change detection manually when needed. 2) Cannot think of any problem right now 3) Yes

codecov[bot] commented 5 years ago

Codecov Report

Merging #28 into master will decrease coverage by 0.76%. The diff coverage is 26.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   31.69%   30.92%   -0.77%     
==========================================
  Files           9        9              
  Lines         407      443      +36     
  Branches       39       42       +3     
==========================================
+ Hits          129      137       +8     
- Misses        278      306      +28
Impacted Files Coverage Δ
...ee-dnd-children/ngx-tree-dnd-children.component.ts 29.41% <ø> (ø) :arrow_up:
.../src/lib/directives/ngx-tree-dnd-drop.directive.ts 35.89% <31.25%> (-4.11%) :arrow_down:
.../src/lib/directives/ngx-tree-dnd-drag.directive.ts 35.89% <33.33%> (-2.57%) :arrow_down:
...x-tree-dnd-parent/ngx-tree-dnd-parent.component.ts 30.23% <9.09%> (-3.54%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4ab693f...134de83. Read the comment docs.

Zicrael commented 5 years ago

Just select "Merge pull request" and press submit.

njofce commented 5 years ago

@Zicrael I think including the option for adding custom template or component instead of node title would make this library very useful. Please let me know in case you are currently working on a similar feature, I am very interested in helping out. Also, I cannot merge this pull request since I have no write access to the repo.

Zicrael commented 5 years ago

I invite you to collaboration this repo, you can merge now. Yes, I just started thinking about change HTML insdie node title instead of simple text.

njofce commented 5 years ago

@Zicrael I was thinking about providing the possibility for adding custom components (dynamic components) as tree nodes. I would like to hear what do you think about this, or what's your preferred approach?

Zicrael commented 5 years ago

@njofce You mean for example pass users components inside tree nodes ( child components )?

njofce commented 5 years ago

@Zicrael Yes, that's what I mean. The only overhead in this case would be that the user will have to provide both display and edit templates in that component, and also will have to provide Input hooks that will allow the library to trigger edit events (ex. we provide an interface to be implemented)

Zicrael commented 5 years ago

Yes, it's a good idea, but what about people who don't need it, or they can't write such a component. Possible solution is to make treeMode: simple || custom in config.

njofce commented 5 years ago

@Zicrael Yes, I think you are right. That might be the best approach for now.

Zicrael commented 5 years ago

Yes, but also what if someone want change just html ( for example put icon inside ) , this user does not need a new component as for me. So we can make another option with html option inside tree. But how to implement editing and adding new children in this case ...

njofce commented 5 years ago

@Zicrael That could be challenging, however I think that the templates for display and add/edit must be included

Zicrael commented 5 years ago

@njofce i revert 2 last reqests because i found huge problem.. After dragging an item, it stops tracking changes, and errors are added to the console. First you need to figure out what the problem is and fix current issues.

njofce commented 5 years ago

@Zicrael I cannot reproduce this error. Can you please let me know of the error message you are receiving?

Zicrael commented 5 years ago

Can you create your own branch, like njoce-branch and make this two PR in njoce-branch fixes branch?

njofce commented 5 years ago

@Zicrael Sorry, haven't been able to follow up with the new branch, as I have been busy lately. Will do it as soon as I can.