Sankey fix for 4+ multilinks #1934
Conversation
monfera
force-pushed
the
sankey-multilink
branch
from
441ee32 to
7c179ae
Compare
monfera
force-pushed
the
sankey-multilink
branch
4 times, most recently
from
93b6100 to
da7ff49
Compare
monfera
force-pushed
the
sankey-multilink
branch
from
da7ff49 to
6ae3b70
Compare
monfera
changed the title
src/traces/sankey/render.js
Outdated
| @@ -155,7 +155,7 @@ function linkModel(uniqueKeys, d, l) { | |||
| var tc = tinycolor(l.color); | |||
| var basicKey = l.source.label + '|' + l.target.label; | |||
| var foundKey = uniqueKeys[basicKey]; | |||
| uniqueKeys[basicKey] = (foundKey === void(0) ? foundKey : 0) + 1; | |||
| uniqueKeys[basicKey] = (foundKey === void(0) ? 0 : foundKey) + 1; | |||
There was a problem hiding this comment.
@monfera thanks for updating the baseline to 🔒 this fix down 🎉
For documentation purposes, can you explain what's going on this line?
There was a problem hiding this comment.
The code relies on a unique key for the purpose of entering / exiting the proper links. A simple solution would be to lean on the index for this. But it's better to associate semantically meaningful elements, e.g. some specific node or link, with the DOM element - useful for future possible animations or tweening. As we don't require an unique identifier array from users, an approximation is
- for nodes: the node label
- for links: the labels of the nodes they link
As there can be more than one link between two arbitrary nodes, the data binding key is made unique by a lookup that generates a new key for additional links. The ternary got flipped during an unrelated refactoring and the lookup came up with a NaN suffix repeatedly which made the data binding to swallow elements over 3 multilinks, considering them identical.
There was a problem hiding this comment.
might be clearer if we just use the fact that foundKey will always be truthy if it exists?
uniqueKeys[basicKey] = (foundKey || 0) + 1;
var key = basicKay + (foundKey ? '' : ('__' + foundKey));
Which brings up: it might be very uncommon, but what if the user actually had a label like 'name' and another like 'name__1'? I could see this popping out of some upstream disambiguation... Wouldn't it be more robust to always add the suffix like:
var keyCounter = uniqueKeys[basicKey] || 0;
uniqueKeys[basicKey] = keyCounter + 1;
var key = basicKey + '_' + keyCounter;
There was a problem hiding this comment.
@alexcjohnson yes, the 1st suggestion works out if we 1-index things as you do, and indeed there is a small risk of name collision - commit on the way
|
Nice, looks good to me! 💃 |
Purports to fix #1886