Skip to content

scatter marker color gradients #1620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 3, 2017
Merged

scatter marker color gradients #1620

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
541460a
scatter marker color gradients
alexcjohnson Apr 22, 2017
ff6c910
Merge branch 'master' into gradient-fill
alexcjohnson May 2, 2017
105ce9b
include gradient arrays in scattercarpet test image
alexcjohnson May 2, 2017
a074259
:hocho: uid and *src from image mocks
alexcjohnson May 2, 2017
1b0a6bb
more robust no-gradient-object fallback
alexcjohnson May 2, 2017
d1c6b51
test gradient restyle behavior
alexcjohnson May 2, 2017
e3af5eb
link gradient to the point, not the color combo
alexcjohnson May 2, 2017
ec4d9ee
store index with each cd item instead of trying to get it from the join
alexcjohnson May 3, 2017
21650c7
ignore cd[i].i in unrelated tests
alexcjohnson May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/components/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ drawing.symbolNumber = function(v) {
return Math.floor(Math.max(v, 0));
};

function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine, gd, i) {
function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine, gd) {
// only scatter & box plots get marker path and opacity
// bars, histograms don't
if(Registry.traceIs(trace, 'symbols')) {
Expand Down Expand Up @@ -292,7 +292,7 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
else gradientColor = markerGradient.color;

var gradientID = 'g' + gd._fullLayout._uid + '-' + trace.uid;
if(perPointGradient) gradientID += '-' + i;
if(perPointGradient) gradientID += '-' + d.i;

sel.call(drawing.gradient, gd, gradientID, gradientType, fillColor, gradientColor);
}
Expand Down Expand Up @@ -361,10 +361,10 @@ drawing.initGradients = function(gd) {
gradientsGroup.selectAll('linearGradient,radialGradient').remove();
};

drawing.singlePointStyle = function(d, sel, trace, markerScale, lineScale, gd, i) {
drawing.singlePointStyle = function(d, sel, trace, markerScale, lineScale, gd) {
var marker = trace.marker;

singlePointStyle(d, sel, trace, markerScale, lineScale, marker, marker.line, gd, i);
singlePointStyle(d, sel, trace, markerScale, lineScale, marker, marker.line, gd);

};

Expand All @@ -378,8 +378,8 @@ drawing.pointStyle = function(s, trace) {
var lineScale = drawing.tryColorscale(marker, 'line');
var gd = Lib.getPlotDiv(s.node());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it might be better to add a ref to those gradient defs in fullData[i] then to look for gd? Or even better refactor those Drawing method to expect fullLayout as argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it for now - I agree this is suboptimal but neither of these alternatives seems particularly simpler to me. If I had to pick one I'd probably go for the first actually... stashing the <defs> in each trace could come in handy elsewhere too.


s.each(function(d, i) {
drawing.singlePointStyle(d, d3.select(this), trace, markerScale, lineScale, gd, i);
s.each(function(d) {
drawing.singlePointStyle(d, d3.select(this), trace, markerScale, lineScale, gd);
});
};

Expand Down
3 changes: 3 additions & 0 deletions src/traces/scatter/arrays_to_calcdata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ var Lib = require('../../lib');
// arrayOk attributes, merge them into calcdata array
module.exports = function arraysToCalcdata(cd, trace) {

// so each point knows which index it originally came from
for(var i = 0; i < cd.length; i++) cd[i].i = i;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard to my comment earlier today about not using calcdata as much: I needed to put this in because what I first tried to do, getting the index from the selection .each(d, i), is not reliable, as evidenced by the scattercarpet test image: the 0th point on the plot was actually index 1 from the trace (point 0 is cut off), but then the legend thought IT was point 0 so overwrote the other point 0 gradient.

BUT once we have that, we could get rid of the rest of these Lib.mergeArray statements - which, to address your concern earlier, don't do any sanitization anyway, we only do that for the axis data - and just directly access the arrays (if they are arrays) from within the style routines.

I'm not going to do that now, but could be a nice 🚀 at some point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👁️ here and nice fix. But, I think the solution would be to make Lib.mergeArray sanatize those arrays instead of 🔪 it. We can debate this in a future issue / PR 😄


Lib.mergeArray(trace.text, cd, 'tx');
Lib.mergeArray(trace.hovertext, cd, 'htx');
Lib.mergeArray(trace.customdata, cd, 'data');
Expand Down
4 changes: 2 additions & 2 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,11 +430,11 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, '');
var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line');

join.each(function(d, i) {
join.each(function(d) {
var el = d3.select(this);
var sel = transition(el);
Drawing.translatePoint(d, sel, xa, ya);
Drawing.singlePointStyle(d, sel, trace, markerScale, lineScale, gd, i);
Drawing.singlePointStyle(d, sel, trace, markerScale, lineScale, gd);

if(trace.customdata) {
el.classed('plotly-customdata', d.data !== null && d.data !== undefined);
Expand Down