Sankey default link label

[monfera]

Fixes #2014

[monfera]

Changes

to

[alexcjohnson]

🐎 (also for link.color) we're always doing this map even if we don't need the default. Would it be worth either:

• running that after the coerce call, ie:

var linkLabel = coerce('link.label');
if(!linkLabel) traceOut.link.label = traceOut.link.value.map(function() {return '';});

• building into coerce that you can pass a function as dflt and it will only be called if needed?

coerce('link.label', function() { return traceOut.link.value.map(function() {return '';}); });

thoughts?

[alexcjohnson]

Also typically when we rely on a previously coerced value to modify later behavior, usually we just save it from its coercion: var linkValue = coerce('link.value')

Which brings up the question: some of these inputs are strictly necessary in order to have meaningful output. What's the minimal set here, without which we should automatically hide the trace? If there's no link.value this map will throw an error, which we definitely don't want.

[alexcjohnson]

Oh I guess you default to [] so it won't error... but it still doesn't seem meaningful right?

[monfera]

Good point, definitely worth avoiding useless work. Whenever adding some array attributes, I felt it'd be good to be able to specify a default value for the array elements (in attributes.js) rather than just the whole array. But your suggestion is more flexible because it allows a function (default element values depending on other values) so I'll

[etpinard]

@monfera why don't you move this piece of logic to the calc step?

[monfera]

@etpinard I think of array values as conceptually default-able (though there's no elementDefault in attributes.js), and saw usages elsewhere that were also in defaults.js. But I can do it in calc or even in the tooltip caller code linkLabel || '' - after all, only the tooltip has a problem with undefined, which is external to sankey. As @alexcjohnson had an alternative suggestion, I wonder which direction would be best in this case.

[alexcjohnson]

Ah, if you can handle it entirely at the point of hover - so no fallback processing needs to happen at all during plotting - that seems like the best of all, performance-wise, and we do have other examples of arrays that may or may not exist and we leave it until the value is needed to test this rather than trying to fill it in at supplyDefaults or even calc.

[monfera]

The fix got pivoted to this consensus, the assertLabel test function now also checks whether the value no longer breaches the tooltip bounding box for a new test case (and other tests that use it already). The test properly breaks when I comment out the fix.

[monfera]

The commit is in, that's all there is to it, plus the tests 😄

[alexcjohnson]

Super - thanks for the quick turnaround! 💃