Generate treemap on child node of hierarchy #52
Conversation
|
This seems reasonable for this treemap implementation, but it also feels like it makes it more likely that at least one of the hierarchical layouts will fail to handle non-root nodes correctly in the future. (For instance, the pack layout checks node.parent to determine whether to translate nodes; if you pass a non-root node to the pack layout, you’ll probably get NaN positions for the nodes.) I think if you want to support this then we’d need tests for all the layouts, and not just d3.treemap. But I also worry that this makes the implementations harder than an alternative approach. I wonder if it would be better to have a method on node to extract a subtree (say, node.subtree, or even changing the definition of node.copy so that it returns a detached subtree when called on a non-root node, which I think would be reasonable although might be strictly considered non-backwards-compatible), so that you could then pass that subtree to any hierarchical layout. You could then also assert that any node passed to a hierarchical layout is a root node, but generally speaking I rely on documentation rather than runtime assertions to enforce correct API usage, and the documentation is already fairly clear that layouts expect root nodes. Though I suppose it could be construed as an informal “root”, as in the top-most node from the perspective of the layout, and not strictly in terms of node.parent = null and node.depth = 0. That could be clarified, too… |
|
It looks like that is how node.copy is implemented. Which is handy, makes intuitive sense, and gets me there. One issue with this copy approach that I'm seeing, though, is that I'm using resquarify to preserve the arrangement of nodes between updates. As a result, although I can copy the child node and hang on to that copy, as opposed to copying on every update, the copy is now orphaned from the larger hierarchy. So the first line of my update function is (I confess I've never used the pack layout, let alone looked at its implementation, so I'm speaking from a more limited perspective.) |
|
Yeah, I realized that sometime after making the above comment; I added another comment to node.copy to make the current behavior slightly more explicit. I’m sorry it’s a tad awkward, but I think that’s the best current solution, and is safer than partial support for hierarchy nodes accepting non-root nodes as input. |
This PR enables treemap(data) to accept a child node of a hierarchy by removing the assumption that the starting depth is 0.
Encountered during the creation of a mostly frivolous bar chart of treemaps example, but perhaps there are other use cases where a treemap is nested inside something else that also wants to take advantage of d3.hierarchy features.
Test included. Thanks for D3.