Issue 4 - Implementation of cosmoz-tree logic #8

JaySunSyn · 2017-07-22T18:18:37Z

Needs to be synced with Neovici/cosmoz-tree#1

nomego · 2017-07-22T19:27:11Z

bower.json

@@ -20,7 +20,8 @@
    "iron-flex-layout": "^1.0.0",
    "cosmoz-i18next": "neovici/cosmoz-i18next#master",
    "cosmoz-dialog": "neovici/cosmoz-dialog#master",
-    "iron-list": "PolymerElements/iron-list#^2.0.4"
+    "iron-list": "PolymerElements/iron-list#^2.0.4",
+    "cosmoz-treenode": "Neovici/cosmoz-treenode"


why was this added?

No response to this.
I think this is an unnecessary dep/import.

@nomego Ok, not sure I get this and I think its related with the behavior I created.

Do I understand correctly, cosmoz-treenode-navigator gets a tree from outside the element? If so, how can I be sure that certain methods of Cosmoz.DefaultTree are implemented in the tree data the navigator gets from outside in order to work with them inside of the navigator? Do we define, the data (tree object) we get from outside needs do be a Cosmoz.DefaultTree (or derived from) object?

My understanding was, we get "node-like" data from outside and use the Cosmoz.Tree or DefaultTree methods inside the navigator. => wrong?

Ok I see the mistake now, cosmoz-tree (or nothing) instead of cosmoz-treenode. Sorry for that, but above question still valid.

Yes, the navigator gets a Cosmoz.Tree from outside the element.
It can be a Cosmoz.DefaultTree or some other implementation.
Cosmoz.Tree acts as an "abstract class" and defines methods that the implementations need to implement.

Yes the last part is wrong, we expect the tree to be created for us.

Also, it's a weird place to comment about this. This change is to import the "cosmoz-treenode" element which we haven't really touched during this PR, so I don't see why the dependency was added.
Ah, now I realize! :) It's just a typo, it should depend on Neovici/cosmoz-tree#master rather than Neovici/cosmoz-treenode :)

nomego · 2017-07-22T19:32:27Z

cosmoz-treenode-navigator.js

-				}
-			}, this)
+			var nodes = {};
+			nodeList.forEach(function (n) { nodes[n.id] = n; });


this should be a reduce()

nomego · 2017-07-22T19:32:51Z

cosmoz-treenode-navigator.js

+			var nodes = {};
+			nodeList.forEach(function (n) { nodes[n.id] = n; });
+
+			var tree = new Cosmoz.DefaultTree(nodes);


this.tree should be supplied instead of this.data

nomego · 2017-07-22T19:39:52Z

cosmoz-treenode-navigator.js

+
+			var tree = new Cosmoz.DefaultTree(nodes);
+
+			tree.getPath = function (node) {


why is this method created?

nomego · 2017-07-25T20:15:26Z

bower.json

@@ -20,7 +20,8 @@
    "iron-flex-layout": "^1.0.0",
    "cosmoz-i18next": "neovici/cosmoz-i18next#master",
    "cosmoz-dialog": "neovici/cosmoz-dialog#master",
-    "iron-list": "PolymerElements/iron-list#^2.0.4"
+    "iron-list": "PolymerElements/iron-list#^2.0.4",
+    "cosmoz-treenode": "Neovici/cosmoz-treenode"


No response to this.
I think this is an unnecessary dep/import.

nomego · 2017-07-25T20:34:30Z

cosmoz-treenode-navigator.html

@@ -62,7 +64,7 @@ <h3 class="layout horizontal center wrap">
 					<div class="section" hidden$="[[ !_renderSection(_searching, index, dataPlane, node) ]]">[[ node.sectionName ]]</div>
 					<paper-item on-tap="_nodeSelected" class="nodeItem">
 						<span class="flex">[[ node.name ]]</span>
-						<iron-icon hidden$="[[ !hasChildren(node) ]]" icon="icons:arrow-forward" on-click="openNode" data-path$="[[ node.path ]]"></iron-icon>
+						<iron-icon hidden$="[[ !hasChildren(node) ]]" data-path$="[[ node.path ]]" icon="icons:arrow-forward" on-click="openNode" data-path$="[[ node.path ]]"></iron-icon>


data-path was already bound

nomego · 2017-07-25T20:36:16Z

cosmoz-treenode-navigator.js

@@ -15,24 +16,26 @@
 			that component is given
 			 */
 			data: {


I don't think data should be kept but rather just expect a

tree: { type: Cosmoz.Tree }

like https://github.com/Neovici/cosmoz-treenode/blob/master/cosmoz-treenode.html#L45

I guess that answers a big part of my above question.

nomego · 2017-07-25T20:36:29Z

cosmoz-treenode-navigator.js

@@ -15,24 +16,26 @@
 			that component is given
 			 */
 			data: {
-				type: Object
+				type: Object,
+				value: function () { return {} },


return on separate line, missing semicolon

nomego · 2017-07-25T20:37:35Z

cosmoz-treenode-navigator.js

-				level = Object.keys(nodes).map(function (key) {
-					var node = nodes[key];
+		_renderLevel: function (pathLocator, tree) {
+			if (!tree) return;


use curly braces

nomego · 2017-07-25T20:40:48Z

cosmoz-treenode-navigator.js

-			}, this);
-
-			return node;
+		_getNode: function (pathLocator, nodes) {


second argument should be dropped if not used (also from computed property)

nomego · 2017-07-25T20:41:00Z

cosmoz-treenode-navigator.js

-
-			return node;
+		_getNode: function (pathLocator, nodes) {
+			if (!pathLocator) return null;


use curly braces

nomego · 2017-07-25T20:41:55Z

cosmoz-treenode-navigator.js

-
-			return parts;
+		_getTreePathParts: function (pathLocator, tree) {
+			if (!tree) return;


use curly braces (x2), why different return values depending on missing argument?

nomego · 2017-07-25T20:42:31Z

cosmoz-treenode-navigator.js

-				}
-			}, this);
-
+			this._openNodeLevelPath = event.currentTarget.getAttribute('data-path');


why not event.currentTarget.dataset.path ?

nomego · 2017-07-26T11:06:03Z

cosmoz-treenode-navigator.js

+		 * of Cosmoz.TreeBehavior
+		 */
+		_getTreeClass: function (treeClass) {
+			treeClass.prototype.afterFound = function (node, propertyName) {


Any reason why we can't just do the equivalent in the map() of _renderLevel ?
This code both updates an unknown tree implementation (once data is replaced with tree) and it writes to the nodes in the tree.

We can. (just a tiny little bit less efficient but no big deal)
I'm just used to such hook methods I know from js-data.

nomego · 2017-07-26T18:44:44Z

bower.json

+    "iron-ajax": "^2.0.2"
+  },
+  "resolutions": {
+    "iron-ajax": "^1.0.0"


Should not be needed if the dep is on iron-ajax#^1.0.0

nomego · 2017-07-26T18:44:57Z

bower.json

-    "webcomponentsjs": "webcomponents/webcomponentsjs#^0.7.0"
+    "webcomponentsjs": "webcomponents/webcomponentsjs#^0.7.0",
+    "cosmoz-tree": "Neovici/cosmoz-tree",
+    "iron-ajax": "^2.0.2"


nomego · 2017-07-26T18:45:15Z

cosmoz-treenode-navigator.js

@@ -5,7 +5,7 @@
 	Polymer({

 		behaviors: [
-			Cosmoz.TranslatableBehavior
+			Cosmoz.TranslatableBehavior,


nomego · 2017-07-26T18:56:33Z

cosmoz-treenode-navigator.js

-					pathSegment = children;
-				}
-				return node;
+		_normalizeNode: function (nodeObj) {


This method actually doesn't return a formatted node as described and expected by the name but rather an array based on the object keys. It is designed to work on node.children input, but is not used that way.
Keep only the object remapping (1:1, return statement) in this method.
Create a new method for _normalizeNodes() that takes an array as input (and maps it by calling _normalizeNode() for each item). Probably needed when doing searches?
Finally, use _normalizeNodes(tree.getChildren(node)) or even a _normalizeChildren() for cases where you want this code to run as it is now.

nomego · 2017-07-26T18:58:44Z

cosmoz-treenode-navigator.js

+			if (!tree || !pathLocator) {
+				return [];
+			};
+			return this._normalizeNode(tree.getPath(pathLocator));


getPath() returns an array, need _normalizeNodes()
But I don't think we need to normalize these, do we ?

nomego · 2017-07-29T19:51:57Z

cosmoz-treenode-navigator.js

-			data: {
-				type: Object
+			tree: {
+				type: Cosmoz.tree,


nomego · 2017-07-29T19:53:10Z

cosmoz-treenode-navigator.js

 			if (searching) {
-				return this.searchAllBranches(inputValue, openNodeLevelPathParts, renderedLevel, data);
+				var results = tree.getNodeByProperty(this.comparisonProperty, inputValue, false, true, renderedLevel);


This should be searchNodes() ?

weird.. I have this on my local copy..

nomego · 2017-07-29T19:57:41Z

cosmoz-treenode-navigator.js

-
-			return this._sortItOut(level);
+			var n = tree.getNodeByPathLocator(pathLocator),
+				level = tree.getChildren(n) ? _objectValues(tree.getChildren(n)) : n;


This could call _objectValues() twice?
Rather just do:

var node = tree.getNodeByPathLocator(pathLocator), children = tree.getChildren(node), level = children || node; return this._sortNodes(this._normalizeNodes(level));

With the changes from the comments in the cosmoz-tree PR.

nomego · 2017-07-29T19:58:09Z

cosmoz-treenode-navigator.js

-					pathSegment = children;
-				}
-				return node;
+		_normalizeNodes: function (nodes) { 


We never needed to normalize a single node?

I dropped the key property (which is getting lost when converting to object values) as it wasn't needed and now we can just call it on a resulting array.

nomego · 2017-07-29T20:05:03Z

cosmoz-treenode-navigator.js

+			if (!tree || !pathLocator) {
+				return [];
+			};
+			return this._normalizeNodes(tree.getPath(pathLocator));


Why do the nodes in the path need to be normalized?
Can't we refactor away _getTreePathParts() completely?

We need node.pathin the path in order to open the node when clicked on a path part.

@nomego We could of course use node.pathLocator but it's nicer to have the same node structure in the view everywhere.

<span class="slash">/</span><a href="#" data-path$="[[ node.path ]]" on-click="openNode">[[ getNodeName(node) ]]</a>

But if you prefer I can do data-path$="[[ node.pathLocator ]]"? instead of returning normalized nodes at _getTreePathParts

Oh, ok, then I see.
Nah, keep it as it is now, it's better.

nomego · 2017-07-29T20:09:05Z

demo/index.html

+		 </div>
+		<script>
+			window.addEventListener('WebComponentsReady', function(e) {
+				document.querySelector('#demo').handleResponse = function(response) {


This could actually be further simplified..

<iron-ajax ... last-response="{{ treeData }}"> <cosmoz-treenode-button-view ... tree="[[ getTree(treeData) ]]">

document.querySelector('#demo').getTree = function (data) { return new Cosmoz.DefaultTree(data); };

Still, I don't think you need window.addEventListener('WebComponentsReady', function(e) { either?

nomego

This looks really good now, but I think that comparisonProperty (searchProperty), childProperty and hasChildren() should move to the cosmoz-tree implementation.

nomego · 2017-07-31T21:02:07Z

cosmoz-treenode-navigator.js

-			return this._sortItOut(level);
+			var node = tree.getNodeByPathLocator(pathLocator),
+				children = tree.getChildren(node),
+				level = children || _objectValues(node);


Why do we need _objectValues() on the node here?
Seems like it would render a weird result?

nomego

Almost done! :)

nomego · 2017-08-01T18:51:59Z

cosmoz-treenode-navigator.js

+			return nodes.map(function (node) {
+				var path = node.pathLocator || node.path;
+				return {
+					name: node[this.comparisonProperty],


this.comparisonProperty should be replaced with this.tree.searchProperty wherever possible.
Same goes for this.childProperty -> this.tree.childProperty.

nomego · 2017-08-02T18:13:43Z

cosmoz-treenode-button-view.html

-				value="{{ value }}" value-path-parts="{{ valuePathParts }}">
+				on-data-plane-changed="refit"
+				highlighted-node-path="{{ highlightedNodePath }}"
+				comparison-property="[[ comparisonProperty ]]"


Drop comparisonProperty here as well

nomego

Looks great! Will merge when cosmoz-tree is ready.

plequang

Minor changes required for _getNode, so that it does not throws exception when the tree property is set asynchronously

plequang · 2017-08-17T11:53:40Z

cosmoz-treenode-navigator.js

@@ -67,7 +63,7 @@

 			chosenNode: {
 				type: Object,
-				computed: '_getNode(value, data)',
+				computed: '_getNode(value)',


The _getNode function needs to be bound to the tree, so that it will be called only when the tree is available.

plequang · 2017-08-17T11:58:16Z

cosmoz-treenode-navigator.js

 		 */
-		_getNode: function (pl, nodes) {
-			if (!pl) {
+		_getNode: function (pathLocator) {


This should be _getNode: function (pathLocator, tree) and ensure tree is defined.

Implemented cosmoz-tree and replaced navigator logic.

a1cd30c

nomego suggested changes Jul 22, 2017

View reviewed changes

JaySunSyn added 2 commits July 23, 2017 18:20

Refactor with Cosmoz.TreeBehavior

e3fc131

fixes of PR comments

24c6455

nomego suggested changes Jul 25, 2017

View reviewed changes

nomego reviewed Jul 26, 2017

View reviewed changes

JaySunSyn added 3 commits July 26, 2017 15:58

PR comments (without rename)

1c1cad1

Replaced "data" with "tree"

61bfdd9

PR comments (back to old naming)

0396b05

nomego suggested changes Jul 26, 2017

View reviewed changes

Fixed _normalizeNode(s) and getChildren()

2220174

nomego suggested changes Jul 29, 2017

View reviewed changes

JaySunSyn added 3 commits July 31, 2017 17:36

fixed renaming of getPathNodes() and searchNodes()

7999720

getChildren() refactor

c273b2d

minor changes

3e59678

nomego suggested changes Jul 31, 2017

View reviewed changes

utilise hasChildren(); fix _renderLevel()

95c4fbb

nomego suggested changes Aug 1, 2017

View reviewed changes

tree class properties searchProperty & pathLocatorSeparator

e43ca86

nomego reviewed Aug 2, 2017

View reviewed changes

JaySunSyn added 2 commits August 3, 2017 15:09

minor fixes

aed99e5

added test structure

0e892a3

nomego approved these changes Aug 5, 2017

View reviewed changes

nomego requested a review from plequang August 7, 2017 11:10

plequang mentioned this pull request Aug 8, 2017

Issue 4 of cosmoz-treenode-navigator Neovici/cosmoz-tree#1

Merged

plequang requested changes Aug 17, 2017

View reviewed changes

ensure tree is defined on _getNode()

5c36f99

plequang merged commit 06851ce into Neovici:master Aug 17, 2017

plequang mentioned this pull request Aug 23, 2017

Use cosmoz-tree for tree structure management #4

Closed


		var tree = new Cosmoz.DefaultTree(nodes);

		tree.getPath = function (node) {

Issue 4 - Implementation of cosmoz-tree logic #8

Issue 4 - Implementation of cosmoz-tree logic #8

Conversation

JaySunSyn commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySunSyn Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySunSyn Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomego left a comment

Choose a reason for hiding this comment

plequang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySunSyn Jul 26, 2017 •

edited

Loading

JaySunSyn Jul 30, 2017 •

edited

Loading