I've been playing with domkit::Tree, it works well, but I have a couple of initial comments:
1). TreeNode ctor should take a nullable obj
From
new make(Obj obj) { this.obj = obj }
To
new make(Obj? obj := null) { this.obj = obj }
TreeNode has several virtual methods, hasChildren(), children(), etc, which means to enable expandable nodes you need to subclass TreeNode. The subclass can obviously hold what ever data it wants, and for simple implementations nothing else is needed. But the TreeNode ctor forces you to create and pass in some other object - which is not always required.
2).TreeNode.isExpanded() required.
I wanted to add the simple ability of expanding / collapsing branch nodes by clicking on them (and not just the expander icon) but there is (currently) no means to tell what the current expand state is! To toggle a node, it would be nice to do:
tree.expand(node, !node.isExpanded)
My work around is to add the following to the TreeNode subclass:
The next problem in my quest to toggle the expanded state, is that I could click to expand, but I couldn't then click to collapse! Turns out I have to select / expand another node, before collapsing the first.
Turns out it is due to this line in Tree.handleMouseUp():
// short-circuit if already selected
if (sel.items.contains(node)) return
It might be better to let the user short circuit that in their own code?
I'm happy to add my own click handler to the node elem (as this may be more symantically correct) but currently the node elem is internal. Plus if I have 10,000 tree nodes I don't really want to create 10,000 event handlers. I'd rather create one on the tree and have the Tree expose a method to translate an event to a node.
4). domkit.css
This doesn't directly concern the Tree but I thought I'd mention it here none the less.
I feel as if the CSS should be split up into two, one file for applications, and another for components.
I wanted to embed domkit into an existing webpage but found the CSS to have declarations for (unwanted) Google fonts which can't be easily surpressed or overridden, and styles for <body> and <a> tags. Whilst these all undoubtably look great for domkit applications, all I needed were the domkit specific styles.
These fonts and generic styles could be moved into a domkit-app.css and the usual domkit component styles could be bundled into a domkit-components.css.
andyWed 27 Sep 2017
1). TreeNode ctor should take a nullable obj
We require an Obj so Selection.item works (same rule as Table) -- but I have no objection to moving that to a virtual method. But thats a big breaking change so will have to noodle on it.
2).TreeNode.isExpanded() required.
I can add that
3). Select event
That code is correct; what is missing is an onTreeEvent like we have in Table. I can look at adding that.
4). domkit.css
Yes I am aware of this -- and would like todo something. Though most likely I will just remove the font definitions and move that to documentation that it's required (but all sorts of font metrics associated with having the correct font - so its thorny).
SlimerDudeWed 27 Sep 2017
all sorts of font metrics associated with having the correct font
Could domkit then not just switch from px units to font-relative lengths such as em or rem? That way it wouldn't matter what font is used. See When to Use Em vs. Rem.
SlimerDudeThu 28 Sep 2017
We require an Obj so Selection.item works
Given you have to sub-class TreeNode, I would say that Selection.item should be the TreeNode (there's no direct comparison with Table as there is no TableNode). Though I can see how in some cases it'd be nice to have a clean separation between nodes and items. But before I bleat on about user choice...
I have no objection to moving that to a virtual method.
As in making domkit::TreeNode.obj() virtual? Yeah, that would work well.
But thats a big breaking change.
Not if you make default obj in TreeNode ctor to this:
new make(Obj obj := this) { ... }
Then you retain backwards compatibility with existing code that use the obj ctor, and all new code has the ability to simplify tree state by having node == item.
SlimerDudeThu 28 Sep 2017
Oh... and I wanted to reduce the left-padding on the nodes, but this is hard coded from code as a style on the element:
SlimerDude Mon 25 Sep 2017
I've been playing with domkit::Tree, it works well, but I have a couple of initial comments:
1). TreeNode ctor should take a nullable obj
From
new make(Obj obj) { this.obj = obj }To
new make(Obj? obj := null) { this.obj = obj }TreeNode has several
virtualmethods,hasChildren(),children(), etc, which means to enable expandable nodes you need to subclass TreeNode. The subclass can obviously hold what ever data it wants, and for simple implementations nothing else is needed. But the TreeNode ctor forces you to create and pass in some other object - which is not always required.2).
TreeNode.isExpanded()required.I wanted to add the simple ability of expanding / collapsing branch nodes by clicking on them (and not just the expander icon) but there is (currently) no means to tell what the current expand state is! To toggle a node, it would be nice to do:
My work around is to add the following to the
TreeNodesubclass:Bool isExpanded() { typeof.field("expanded").get(this) }3). Select event
The next problem in my quest to toggle the expanded state, is that I could click to expand, but I couldn't then click to collapse! Turns out I have to select / expand another node, before collapsing the first.
Turns out it is due to this line in
Tree.handleMouseUp():It might be better to let the user short circuit that in their own code?
I'm happy to add my own
clickhandler to the node elem (as this may be more symantically correct) but currently the node elem isinternal. Plus if I have 10,000 tree nodes I don't really want to create 10,000 event handlers. I'd rather create one on the tree and have the Tree expose a method to translate an event to a node.4). domkit.css
This doesn't directly concern the
Treebut I thought I'd mention it here none the less.I feel as if the CSS should be split up into two, one file for applications, and another for components.
I wanted to embed domkit into an existing webpage but found the CSS to have declarations for (unwanted) Google fonts which can't be easily surpressed or overridden, and styles for
<body>and<a>tags. Whilst these all undoubtably look great for domkit applications, all I needed were the domkit specific styles.These fonts and generic styles could be moved into a
domkit-app.cssand the usual domkit component styles could be bundled into adomkit-components.css.andy Wed 27 Sep 2017
We require an
ObjsoSelection.itemworks (same rule as Table) -- but I have no objection to moving that to a virtual method. But thats a big breaking change so will have to noodle on it.I can add that
That code is correct; what is missing is an
onTreeEventlike we have in Table. I can look at adding that.Yes I am aware of this -- and would like todo something. Though most likely I will just remove the font definitions and move that to documentation that it's required (but all sorts of font metrics associated with having the correct font - so its thorny).
SlimerDude Wed 27 Sep 2017
Could domkit then not just switch from
pxunits to font-relative lengths such asemorrem? That way it wouldn't matter what font is used. See When to Use Em vs. Rem.SlimerDude Thu 28 Sep 2017
Given you have to sub-class
TreeNode, I would say thatSelection.itemshould be theTreeNode(there's no direct comparison with Table as there is noTableNode). Though I can see how in some cases it'd be nice to have a clean separation between nodes and items. But before I bleat on about user choice...As in making
domkit::TreeNode.obj()virtual? Yeah, that would work well.Not if you make default
objinTreeNodector tothis:new make(Obj obj := this) { ... }Then you retain backwards compatibility with existing code that use the obj ctor, and all new code has the ability to simplify tree state by having node == item.
SlimerDude Thu 28 Sep 2017
Oh... and I wanted to reduce the
left-paddingon the nodes, but this is hard coded from code as a style on the element:private Void doRefreshNode(TreeNode node) { ... content.style->paddingLeft = "${node.depth*20}px" ...I thought perhaps I could override
refreshNode()and re-apply my own padding - but alasrefreshNode()is not virtual. :(Would this be catered for by
TreeEvent?-= EDIT =-
Got it! I can use
TreeNode.onElem()!!! Any chance thedepthfield could be made public too?andy Thu 5 Oct 2017
These were all addressed -- I will document the changes in the release notes -- ties into some broader changes to domkit