fix(tree, menu): return proper value for positionInParent call; change z-index of the menu

 * added z-index of 999 to prevent right click menu from being covered by other elements
 * implemented lastIndex as second parameter of NodeRemovedEvent (#111)
 * fixed null pointer in positionInParent() for top level node

Author: motabass

src/styles.css

@@ -13,6 +13,7 @@
   background-color: #eee;
   color: #212121;
   font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
+  z-index: 999;
 }
 
 .node-menu .node-menu-content li.node-menu-item {

src/tree.events.ts

@@ -25,7 +25,7 @@ export class NodeMovedEvent extends NodeDestructiveEvent {
 }
 
 export class NodeRemovedEvent extends NodeDestructiveEvent {
-  public constructor(node: Tree) {
+  public constructor(node: Tree, public lastIndex: number) {
     super(node);
   }
 }

src/tree.service.ts

@@ -33,7 +33,7 @@ export class TreeService {
   }
 
   public fireNodeRemoved(tree: Tree): void {
-    this.nodeRemoved$.next(new NodeRemovedEvent(tree));
+    this.nodeRemoved$.next(new NodeRemovedEvent(tree, tree.positionInParent));
   }
 
   public fireNodeCreated(tree: Tree): void {

src/tree.ts

@@ -7,7 +7,8 @@ import {
   omit,
   size,
   once,
-  includes
+  includes,
+  isNil
 } from './utils/fn.utils';
 
 import { Observable, Observer } from 'rxjs/Rx';
@@ -262,6 +263,10 @@ export class Tree {
    * @returns {number} The position inside a parent.
    */
   public get positionInParent(): number {
+    if (this.isRoot()) {
+      return -1;
+    }
+
     return this.parent.children ? this.parent.children.indexOf(this) : -1;
   }
 
@@ -318,7 +323,7 @@ export class Tree {
    * @returns {boolean} A flag indicating whether or not this tree is the root.
    */
   public isRoot(): boolean {
-    return this.parent === null;
+    return isNil(this.parent);
   }
 
   /**
@@ -440,7 +445,7 @@ export class Tree {
     } else if (this.node._foldingType === FoldingType.Expanded) {
       return get(this.node.settings, 'cssClasses.expanded', null);
     } else if (this.node._foldingType === FoldingType.Empty) {
-       return get(this.node.settings, 'cssClasses.empty', null);
+      return get(this.node.settings, 'cssClasses.empty', null);
     }
 
     return get(this.node.settings, 'cssClasses.leaf', null);

test/tree.service.spec.ts

@@ -43,18 +43,30 @@ describe('TreeService', () => {
   it('fires node removed events', () => {
     spyOn(treeService.nodeRemoved$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
     treeService.fireNodeRemoved(tree);
 
     expect(treeService.nodeRemoved$.next).toHaveBeenCalledTimes(1);
-    expect(treeService.nodeRemoved$.next).toHaveBeenCalledWith(new NodeRemovedEvent(tree));
+    expect(treeService.nodeRemoved$.next).toHaveBeenCalledWith(new NodeRemovedEvent(tree, -1));
+  });
+
+  it('fires node removed events witch corretly identified postion removed node used to have in its parent', () => {
+    spyOn(treeService.nodeRemoved$, 'next');
+
+    const child1 = { value: 'Servant#1' };
+    const child2 = { value: 'Servant#2' };
+    const tree = new Tree({ value: 'Master', children: [child1, child2] });
+    treeService.fireNodeRemoved(tree.children[1]);
+
+    expect(treeService.nodeRemoved$.next).toHaveBeenCalledTimes(1);
+    expect(treeService.nodeRemoved$.next).toHaveBeenCalledWith(new NodeRemovedEvent(tree.children[1], 1));
   });
 
   it('fires node moved events', () => {
     spyOn(treeService.nodeMoved$, 'next');
 
-    const parent = new Tree({value: 'Master Pa'});
-    const tree = new Tree({value: 'Master'}, parent);
+    const parent = new Tree({ value: 'Master Pa' });
+    const tree = new Tree({ value: 'Master' }, parent);
 
     treeService.fireNodeMoved(tree, parent);
 
@@ -65,7 +77,7 @@ describe('TreeService', () => {
   it('fires node created events', () => {
     spyOn(treeService.nodeCreated$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
 
     treeService.fireNodeCreated(tree, parent);
 
@@ -76,7 +88,7 @@ describe('TreeService', () => {
   it('fires node selected events', () => {
     spyOn(treeService.nodeSelected$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
 
     treeService.fireNodeSelected(tree);
 
@@ -87,7 +99,7 @@ describe('TreeService', () => {
   it('fires node renamed events', () => {
     spyOn(treeService.nodeRenamed$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
 
     treeService.fireNodeRenamed('Bla', tree);
 
@@ -98,7 +110,7 @@ describe('TreeService', () => {
   it('fires node expanded events', () => {
     spyOn(treeService.nodeExpanded$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
 
     treeService.fireNodeExpanded(tree);
 
@@ -109,7 +121,7 @@ describe('TreeService', () => {
   it('fires node collapsed events', () => {
     spyOn(treeService.nodeCollapsed$, 'next');
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
 
     treeService.fireNodeCollapsed(tree);
 
@@ -118,9 +130,9 @@ describe('TreeService', () => {
   });
 
   it('fires events on which other tree should remove selection', done => {
-    const selectedTree = new Tree({value: 'Master'});
+    const selectedTree = new Tree({ value: 'Master' });
 
-    const tree = new Tree({value: 'Master'});
+    const tree = new Tree({ value: 'Master' });
     treeService.unselectStream(tree)
       .subscribe((e: NodeSelectedEvent) => {
         expect(e.node).toBe(selectedTree);
@@ -134,8 +146,8 @@ describe('TreeService', () => {
     const masterTree = new Tree({
       value: 'Master',
       children: [
-        {value: 'Servant#1'},
-        {value: 'Servant#2'}
+        { value: 'Servant#1' },
+        { value: 'Servant#2' }
       ]
     });
 
@@ -156,12 +168,12 @@ describe('TreeService', () => {
     const masterTree = new Tree({
       value: 'Master',
       children: [
-        {value: 'Servant#1'},
-        {value: 'Servant#2'}
+        { value: 'Servant#1' },
+        { value: 'Servant#2' }
       ]
     });
 
-    const tree = new Tree({value: 'tree'});
+    const tree = new Tree({ value: 'tree' });
 
     const elementRef = new ElementRef(null);
 
@@ -208,8 +220,8 @@ describe('TreeService', () => {
     const masterTree = new Tree({
       value: 'Master',
       children: [
-        {value: 'Servant#1'},
-        {value: 'Servant#2'}
+        { value: 'Servant#1' },
+        { value: 'Servant#2' }
       ]
     });
 
@@ -226,8 +238,8 @@ describe('TreeService', () => {
     const masterTree = new Tree({
       value: 'Master',
       children: [
-        {value: 'Servant#1'},
-        {value: 'Servant#2'}
+        { value: 'Servant#1' },
+        { value: 'Servant#2' }
       ]
     });
 

test/tree.spec.ts

@@ -81,8 +81,8 @@ describe('Tree', () => {
 
   it('should know how to detect Renamable node', () => {
     const renamableNode = {
-      setName: () => {},
-      toString: () => {}
+      setName: () => { },
+      toString: () => { }
     };
 
     const renamableNodeImposter = {
@@ -232,6 +232,59 @@ describe('Tree', () => {
     expect(child.value).toEqual('Master');
   });
 
+  it('has position which equals to -1 when does not have a parent', () => {
+    const fonts: TreeModel = {
+      value: 'Serif  -  All my children and I are STATIC ¯\\_(ツ)_/¯',
+      children: [
+        { value: 'Antiqua' },
+        { value: 'DejaVu Serif' },
+        { value: 'Garamond' },
+        { value: 'Georgia' },
+        { value: 'Times New Roman' },
+      ]
+    };
+
+    const tree = new Tree(fonts);
+    expect(tree.positionInParent).toEqual(-1);
+  });
+
+  it('has position which equals to -1 when still connected to parent but parent already does not have children', () => {
+    const fonts: TreeModel = {
+      value: 'Serif  -  All my children and I are STATIC ¯\\_(ツ)_/¯',
+      children: [
+        { value: 'Antiqua' },
+        { value: 'DejaVu Serif' },
+        { value: 'Garamond' },
+        { value: 'Georgia' },
+        { value: 'Times New Roman' },
+      ]
+    };
+
+    const tree = new Tree(fonts);
+    const child = tree.children[3];
+
+    (tree as any)._children = null;
+    expect(child.positionInParent).toEqual(-1);
+  });
+
+  it('has position which equals to actual position rendered', () => {
+    const fonts: TreeModel = {
+      value: 'Serif  -  All my children and I are STATIC ¯\\_(ツ)_/¯',
+      children: [
+        { value: 'Antiqua' },
+        { value: 'DejaVu Serif' },
+        { value: 'Garamond' },
+        { value: 'Georgia' },
+        { value: 'Times New Roman' },
+      ]
+    };
+
+    const tree = new Tree(fonts);
+    const child = tree.children[4];
+
+    expect(child.positionInParent).toEqual(4);
+  });
+
   it('adds child to a particular position in a tree', () => {
     const fonts: TreeModel = {
       value: 'Serif  -  All my children and I are STATIC ¯\\_(ツ)_/¯',
@@ -311,7 +364,7 @@ describe('Tree', () => {
       children: null
     });
 
-    const addedSibling = tree.addSibling(new Tree({value: 'bla'}));
+    const addedSibling = tree.addSibling(new Tree({ value: 'bla' }));
 
     expect(addedSibling).toBeNull();
     expect(tree.parent).toBeNull();
@@ -729,7 +782,7 @@ describe('Tree', () => {
   it('cannot switch "Empty" folding type', () => {
     const masterTree = new Tree({
       value: 'Master',
-       children: []
+      children: []
     });
 
     expect(masterTree.foldingType).toEqual(FoldingType.Empty);