nodeHeight as a function

[RassaLibre]

Hello,
I thought it might be cool if you could actually specify the nodeHeight as function so I implemented it!

Why?

Sometimes you want the height of the node to be dependent on the inner content because sometimes you want the content to be a long text and sometimes it is just a one line. There is no reason of having all the nodes with a fixed height.

Implementation?

Renderer now accepts the option nodeHeight as a function. This function has one parameter- nodeData. This parameter is equal to what the programmer passes to node.data. This param is used in example as a source of the node's content. I do not pass the entire node because then the programmer would be able to change some of the calculated numbers and we do not want that. This function MUST return a number. If it does not, then an error is shown in the console with a description of what happened and with what nodeData and a fallback value is used so the rendering process is completed without any problems.

I have also added:

1. unit tests
2. example html file + linking from the other example pages
3. explanation of the whole thing in the docs

[kristw]

This is a great idea and thank you for sending in the pull request. I see that you also add examples, docs and tests. Awesome work! I will be happy to include this functionality, but would like to recommend a few changes before doing so.

First of all, add function functor to helper.js. (This was copied from d3.functor.) Functor is a function that:

• given a function, it returns that function
• given a value, it returns a function that returns that value.

functor(v) {
  return typeof v === "function" ? v : function() {
    return v;
  };
}

This little function can help you handle option.nodeHeight a bit more conveniently.

In renderer.js, the code in constructor Renderer(options) that handle options.nodeHeight (line 6 to 20 in the new code) are not needed any more. You can revert to the old code.

Then in getWaypoints (line 66-68 in the new code) where you compute nodeHeight, just call

var nodeHeight = helper.functor(options.nodeHeight)(node.data);

In layout (line 115), you can define

var nodeHeightFn = helper.functor(options.nodeHeight);

and the rest of the code can use nodeHeightFn(node.data) without caring what is the type of options.nodeHeight. For example line 134-139 can be replaced with this one line.

- if(typeof options.nodeHeight === 'function'){
-   node.x = -pos - options.nodeHeight(node.data);
- }
- else{
-   node.x = -pos - options.nodeHeight;
- }
+ node.x = -pos - nodeHeightFn(node.data);

[RassaLibre]

Thank you very much! I have updated the code as you described above. It is a very neat way how to handle the nodeHeight. I agree, mine was a bit fuzzy. I removed old the console.logs + I also extended the functor function in order to check if the return value is a number. If not, then I return the default node height value and I notify the programmer in the console (I think it is better than just silently fall back). I had a nice test for it and it was failing, so I included it there. Just let me know if you have more suggestions :-)

[kristw]

Thanks for adding the check, but a helper function like this should be quite abstract and does not get too involved with how will be used. To be specific, this functor should be useful for other value not just nodeHeight or event limited to Number. This can be used for String, Object, etc. as well. In conclusion, I wouldn't add the check here.

If you really want to add the check, it might be better to do this check in getWaypoints() and layout by checking the output value of nodeHeightFn() regardless of the type of options.nodeHeight. That way you also validate if somebody accidentally set options.nodeHeight to non-Number constant.

The check can print the warning like nodeHeight does not return a Number, returning 10 instead of ${value}.

[kristw]

Thank you for making the change so quickly. While reviewing it, I just thought of another important issue that needs to be address first, gap computation.

The way Labella works is that it distributes labels into layers. Using a top down example, nodes in the first layer should have the same y. Similarly, nodes in the second layer should have same y. By doing so, it only needs to remove overlaps for one axis, x.

Originally, because all nodes have same height, gap was computed from constant options.nodeHeight. With the variance in nodeHeight, gap needs to be recomputed, but it is not a function of individual node's height, it has to be computed based on maximum nodeHeight in every layer. Otherwise nodes in each layer will not line up at the same y. You may have to group nodes first by .getLayerIndex() then compute the max height and use that to compute the gap for each layer.

Additionally, I recommend passing node into nodeHeight() rather than node.data. My reasons are that (1) there are additional information that maybe useful for computing height, such as .getLayerIndex() or .y. For example, you may want to make lower layer shorter/smaller. (2) not passing node in here does not guarantee that user will not modify it somewhere else anyway.

[RassaLibre]

Yeah, I agree that the check is very specific and the helpers should be very abstract. Did not thought about that. Damn, there is still a lot I need to learn haha. I removed it and I will fix the gaps and passing the entire node instead of nodeData tomorrow (I am located in Norway and it is 22:32 here atm.). I agree that the user can change it anywhere else. That is what I btw like about the implementation- it is very open. You do not need to dig in properties to get the list of nodes.

[kristw]

Take your time. There is no rush on this. Really appreciate your help.

[RassaLibre]

I think we need something like this:

case 'down':
  nodes.forEach(function(node, index){
    var pos = 0;
    if(gaps){ //if the nodeHeight is a function
      var layerIndex = node.getLayerIndex();
      for(var i = 0; i < layerIndex; i++){
        pos += gaps[i];
      }
      pos += options.layerGap;
    }
    else{
      pos = node.getLayerIndex() * gap + options.layerGap;
    }
    node.x = node.currentPos;
    node.y = pos;
    node.dx = node.width;
    node.dy = nodeHeightFn(node);
  });
  break;

gaps is an array which contains the max height in the layer + options.layerGap. At least this solved my problem of overlapping labels. I just need to do a bit more changes here an there so the paths are generated correctly. I will then post the changes in a commit and you can take a look :)

[kristw]

A little tweak from the code above so yPos is only computed once instead of for every node. Haven't tested but something along this line. For the path correction you have to modify getWayPoints.

var numLayers = ... // max of node.getLayerIndex() + 1; 

case 'down':
  var yPos;
  if(gaps){
    yPos = gaps.reduce(function(prev, current){ 
      prev.push(current + prev[prev.length-1]));
      return prev;
    }, [options.layerGap]);
  }
  else{
    yPos = [];
    for(var i=0;i<numLayers;i++){
      yPos.push(i*gap + options.layerGap);
    }
  }

  nodes.forEach(function(node){
    node.x = node.currentPos;
    node.y = yPos[node.getLayerIndex()];
    node.dx = node.width;
    node.dy = nodeHeightFn(node);
  });
  break;

[RassaLibre]

I find my version a bit more readable but also a bit dumb :-D I have managed to tweak the getWayPoints function and it seems to be working. Will commit the changes soon :-)

[RassaLibre]

Hei Krist,
sorry for getting back to you that late. I realised that it gets quite complex if one wants to implement this the right way. Since we dropped Labella from our project I can not work on it in my working hours so I have to look at it in my spare time and that might take a while. It is up to you if you want to close this pull request or leave it open. I will see what I can do and if I figure this out, I will get back to you. I am sorry for that :-)

[kristw]

No problem. Thank you for bringing up the idea in the first place. We can leave this open to keep track (and you already made some progress). If anybody decide to do something with it, just add the comment so others will know. No pressure to do anything though. Thank you again for your voluntary work.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Tomas Prochazka
❌ RassaLibre

---

Tomas Prochazka seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}