#2641 domkit::Tree

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


new make(Obj obj) { this.obj = obj }


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:

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():

// 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.

andy Wed 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).

SlimerDude Wed 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.

SlimerDude Thu 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.

SlimerDude Thu 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:

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 alas refreshNode() is not virtual. :(

Would this be catered for by TreeEvent?

-= EDIT =-

Got it! I can use TreeNode.onElem()!!! Any chance the depth field 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

Login or Signup to reply.