Pass customdata through to scatter calcdata #1379
Conversation
|
Looks reasonable to me. Do you also want to:
|
|
I've added a |
|
Nice. Yes, I think it'd be worthwhile to avoid the loop in the (overwhelmingly common) case of no |
| var expected = [false, false, true, true, true, true, false]; | ||
|
|
||
| points.each(function(cd, i) { | ||
| expect(d3.select(this).classed('plotly-customdata')).toBe(expected[i]); |
There was a problem hiding this comment.
Beautiful test. I agree, end-to-end is the right way to test this particular beast.
src/traces/scatter/style.js
Outdated
| pt.call(Drawing.pointStyle, d.trace || d[0].trace); | ||
|
|
||
| pt.each(function(cd) { | ||
| d3.select(this).classed('plotly-customdata', cd.data !== null && cd.data !== undefined); |
There was a problem hiding this comment.
Just a little style point: pt makes it sounds like it refers to a single point, but it's all points so I'd change it to pts. And elsewhere cd refers to a whole calcdata array so this tripped me up for a sec - maybe change it to pointData or something.
|
Caveat I just can't ignore: animation. Going to add that to the test to ensure these are properly updated. Since transitions don't work through the style pass, an animation update won't update these. Fixing. This might be reason enough to move this to the plot step instead. |
|
Okay, I've updated the PR (in a test-driven fail-first manner that actually tests what it claims to 👍). It unsets customdata via an animation that doesn't replot in order to verify that it's actually set/unset/updated correctly. This required moving it to |
|
Very nice. 💃 |
This PR adds a
customdataattribute to scatter traces that is passed through and assigned to thedataproperty of the corresponding calcdata entry.To do: verify the manner in which this is (or is not?) actually passed all the way through to the dom element.
ping @alexcjohnson @etpinard