Legend Marker Vertical Alignment

[antoinerg]

Closes #2798 (never mind the issue number in the branch's name which is wrong!)

[etpinard]

Nice use of lineHeight 🎉

[etpinard]

Can you add another mock testing valign: "middle" ?

[antoinerg]

Absolutely! Done in a02910a

[etpinard]

... and let's add one jasmine test 🔒 ing down relayout for 'valign'

[antoinerg]

Nice catch! Thank you so much! Test can be found in a375c99

[antoinerg]

Fix bug when valign equals middle. Because markerOffsetY is zero when at middle position, I had to change this line.

[etpinard]

Hmm. But 'middle' is the default value isn't it? Why didn't our tests catch sooner?

[antoinerg]

All the mocks were rendered properly because the transform attribute was not added. However, if there's already a transform attribute on the svg group because valign was top, then it needs to be removed on relayout if valign is switched back to middle.

[etpinard]

However, if there's already a transform attribute on the svg group because valign was top, then it needs to be removed on relayout if valign is switched back to middle.

OK this makes sense. I don't understand how the transform gets removed and why this patch fixes the bug though. Boolean(NaN) is false so the old and this new line look equivalent to me.

[etpinard]

Wouldn't something like

var valign = gd._fullLayout.valign;

if(valign === 'middle') {
  layers.attr('transform', null); // this here is a fun d3 trick to unset DOM attributes
} else {
  var factor = {top: 1, bottom: -1}[valign];
  var markerOffsetY = factor * (0.5 * (d[0].lineHeight - d[0].height + 3));
  layers.attr('transform', 'translate(0,' + markerOffsetY + ')');
}

be what we're looking for?

[antoinerg]

Your solution would work except that we should check whether markerOffsetY is a number. It might not be when this function is visited and errors would be thrown in the console because of garbage translate(0, NaN). This is why I added if(!isNaN(markerOffsetY))!

[antoinerg]

I'd have to check to make sure but I think it's called on first draw to figure out how big the legend will be. At this point, d[0].lineHeight d[0].height are not defined and we get a NaN. Although everything renders correctly in the end, we get the following in the console:
d3.js:663 Error: <g> attribute transform: Trailing garbage, "translate(0,NaN)".

[antoinerg]

That is indeed what is happening which is why if(!isNaN(markerOffsetY)) is required.

[etpinard]

Ok, in this case if we know that lineHeight/height are sometimes undefined, we should not even attempt to compute markerOffsetY. So something like:

var valign = gd._fullLayout.valign;
var lineHeight = d[0].lineHeight;
var height = d[0].height;

if(valign === 'middle' || !lineHeigth || !lheight) {
  layers.attr('transform', null); // this here is a fun d3 trick to unset DOM attributes
} else {
  var factor = {top: 1, bottom: -1}[valign];
  var markerOffsetY = factor * (0.5 * lineHeight - height + 3));
  layers.attr('transform', 'translate(0,' + markerOffsetY + ')');
}

would make that clearer.

[antoinerg]

In commit c1b871d, I reused most of the above 😄 !

[etpinard]

Just noticed this. Using Drawing.getTranslate would be cleaner.

[antoinerg]

Thanks! 🔍

Done in c1b871d!

[etpinard]

Nicely done 💃