Skip to content

Commit

Permalink
Fixed #9991 - First node dropped into empty tree not triggering onNod…
Browse files Browse the repository at this point in the history 
…eDrop
  • Loading branch information
@yigitfindikli
yigitfindikli committed Mar 12, 2021
1 parent 986e606 commit d9efb83
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions src/app/components/tree/tree.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ export class UITreeNode implements OnInit {
originalEvent: event,
dragNode: dragNode,
dropNode: this.node,
dropIndex: this.index,
index: this.index,
accept: () => {
this.processPointDrop(dropParams);
}
Expand All @@ -218,7 +218,7 @@ export class UITreeNode implements OnInit {
originalEvent: event,
dragNode: dragNode,
dropNode: this.node,
dropIndex: this.index
index: this.index
});
}
}
Expand Down Expand Up @@ -1032,6 +1032,16 @@ export class Tree implements OnInit,AfterContentInit,OnChanges,OnDestroy,Blockab
let dragNodeIndex = this.dragNodeIndex;
this.dragNodeSubNodes.splice(dragNodeIndex, 1);
this.value = this.value||[];

if (this.value.length === 0) {
this.onNodeDrop.emit({
originalEvent: event,
dragNode: dragNode,
dropNode: null,
index: dragNodeIndex

This comment has been minimized.

Sign in to view

Copy link
@leifjones

leifjones Apr 10, 2021

Aha! Re: #10093, it seems this is missing the accept() method.

This comment has been minimized.

Sign in to view

Copy link
@leifjones

leifjones Apr 10, 2021

However, some refactor would definitely be needed because just adding

                    accept: () => {
                        this.processPointDrop(dropParams);
                    }

wouldn't seem to work because processPointDrop() is a method of the UITreeNode class. Perhaps that method could be made accessible, or replicated in this class?

This comment has been minimized.

Sign in to view

Copy link
@yigitfindikli

yigitfindikli Apr 10, 2021

Author Contributor

Hi @mrsegen,
Thanks for your feedback. We'll review #10093 on 11.4.0.

This comment has been minimized.

Sign in to view

Copy link
@hoberlaender

hoberlaender Aug 6, 2021

For my point of view it is a mistake to delete the attribute 'dropIndex' of the emitted object in the function 'onDropPoint' and change it to 'index'. So it is not possible to distinguish between the actions 'drop on a node' and 'drop between two nodes'. We have to use primeNG Version 11.3.1 but using Angular Version 12 because of that change.Can you explain why the change is done? Thank you in advance. Regards, Heiko

This comment has been minimized.

Sign in to view

Copy link
@leifjones

leifjones Aug 7, 2021 •
edited
Loading

@hoberlaender
I don’t have an answer to your question at the end. But I found a workaround (in a context in which being an item in the general list means it’s a child of a main parent node, and in which leaves may not contain nodes):
It involves a couple booleans:

const potentialChild: TreeNode = event.dragNode;
const droppedDirectlyWithinList: boolean = event.dropNode === null; // meaning either in an empty list or between nodes
let potentialParent: TreeNode;
if (droppedDirectlyWithinList === true) { potentialParent = mainParent; }
else { potentialParent = event.dropNode; }
const potentialParentIsLeaf: boolean = potentialParent.leaf === true;
if (!!potentialParentIsLeaf && !! droppedDirectlyWithinList) {
  if (potentialParent.parent) { potentialParent = potentialParent.parent }
  else { potentialParent = mainParent }
}
// some validation for our context
if (!potentialParent.children) { potentialParent.children = []; }
potentialParent.children.push(potentialChild);
// more customization of node template and dragable status for our context

Message back with any questions
})
}

this.value.push(dragNode);

this.dragDropService.stopDrag({
Expand Down

6 comments on commit d9efb83

@hoberlaender
Copy link

Choose a reason for hiding this comment

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

Thank you for the replay.
For my point of view executing the line "const droppedDirectlyWithinList: boolean = event.dropNode === null;" never set droppedDirectlyWithinList to "true" because event.dropNode will never be undefined or null.

My problem is, that if a node is dropped between two nodes, in "primeng-tree.js the function "onDropPoint" (Line 72) is executed and if a node is dropped on a node, the function "onDropNode" (Line 177) is executed. Thats okay. But I don't find a way to distinguish between these two situations. Both functions emit:

this.tree.onNodeDrop.emit({
originalEvent: event,
dragNode: dragNode,
dropNode: this.node,
index: this.index
});

There is no difference and so I'm not able to check if I have to place the dragged node above the dropNode at the same level or inside the dropNode als new children (leaf).

In Version 11 there was the difference using the attributes of the emitted event "index" and "dropIndex".

Thank you in advance for your help. Regards, Heiko

@leifjones
Copy link

@leifjones leifjones commented on d9efb83 Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@hoberlaender Aha. I believe I’m missing a subtleties but I follow your point. From prior experience with filing bugs for this repo, it’s more likely that the team will be able to address the issue if you file an issue here: https://github.com/primefaces/primeng/issues

If at all possible, try to reproduce it with sample code, e.g. this issue: #10093 , so there is a clear, concrete problem for the developers to resolve. Or, if you can think of a way to modify things, feel free to put up a pull request - even a rough draft. If it’s not accepted, it would likely at least spur conversation or ideas towards a solution.

And some of the time, when I attempt to demonstrate that something can’t be done, I wind up finding out there is a way

@hoberlaender
Copy link

Choose a reason for hiding this comment

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

Thank you again for the replay.

I found a workaround to face my problem. Right at the beginning of drop function I examine the delivered "orginalEvent". There is the property "target" which holds the information about the HTML element which is the target of the HTML drop event. So I can distingush between the HTML element between two nodes (HTMLLIElement) and a node (HTMLDIVElement). If a node is dropped between two nodes I add "dropIndex" property to the event object with the value of "index". After that the logic can stay unchanged like using PrimeNG Version 11.3.1.

if (event.originalEvent == null || event.originalEvent.target == null) {
  return;
}

const target = event.originalEvent.target.toString();

if (target === "[object HTMLLIElement]") {
  event.dropIndex = event.index;
}

...

So I don't start an issue for now.

Thank you for your help. Kind Regards, Heiko

@leifjones
Copy link

@leifjones leifjones commented on d9efb83 Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for sharing that workaround!!

Another approach could be to omit the .toString() and to replace the conditional with type of target === 'HTMLLIElement'

@hoberlaender
Copy link

Choose a reason for hiding this comment

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

Thank you for sharing your approach, it is much better.
I will change my code.

Kind Regards, Heiko

@hoberlaender
Copy link

Choose a reason for hiding this comment

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

I tried your approach but there is a problem:

typeof event.originalEvent.target returns always 'object' so there is no chance to distingush between the types of HTML elements.

Kind Regards, Heiko

Please sign in to comment.