Skip to content

Selection re-ordering for scatter traces with ids #1709

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 5 commits into from
May 23, 2017
Merged

Selection re-ordering for scatter traces with ids #1709

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbcec26
add failing test cases showcasing issue #1706
etpinard May 19, 2017
83234b5
[breaking] add `join.order()` calls during marker and text node updates
etpinard May 19, 2017
c8c3197
rm Drawing.pointStyle and Drawing.translatePoints on marker enter sel
etpinard May 23, 2017
5fa1e97
add test case for nodes add/remove on redraw:false animations
etpinard May 23, 2017
9c35675
do not remove nodes in Drawing.translatePoints
etpinard May 23, 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
7 changes: 4 additions & 3 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,9 +419,6 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var enter = join.enter().append('path')
.classed('point', true);

enter.call(Drawing.pointStyle, trace)
Copy link
Contributor Author

@etpinard etpinard May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@n-riesco thanks very much 🎉

Your solution appears to work:
peek 2017-05-23 11-43

My only concern now is how this interacts with animations. I think the reason why translatePoints and pointStyle are called on enter selection and merge selection has to do with this block below where entering nodes have their opacity transitioned from 0 to 1. I suspect this commit will break this behavior (although the tests are passing) has the entering aren't positioned and styled properly before opacity transition cc @rreusser . It would be nice to write a test case here 😏

I'm thinking instead, we could keep those enter.call(), make Drawing.translatePoints not (and maybe never) remove nodes and make the merge selection .each block below (which comes after the .order()) handle the cases where nodes need to be removed.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point you make about animations is important.
And the solution you propose (i.e. remove nodes only after we're done with the enter selection) is more robust.
👍 from me.

Copy link
Contributor

@n-riesco n-riesco May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard after thinking a little bit about it. I'm still think that translatePoints and pointStyle should be applied to the merge selection to keep in sync DOM and trace.

I haven't tested it, but .order should take care of my concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still think that translatePoints and pointStyle should be applied to the merge

That's always been the case here and here.

.call(Drawing.translatePoints, xa, ya, trace);

if(hasTransition) {
enter.style('opacity', 0).transition()
.style('opacity', 1);
Expand All @@ -430,6 +427,8 @@ 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.order();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need join.order() twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one for marker nodes, one for text nodes

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah right - didn't notice join got reassigned between 👍


join.each(function(d) {
var el = d3.select(this);
var sel = transition(el);
Expand Down Expand Up @@ -460,6 +459,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
// it gets converted to mathjax
join.enter().append('g').classed('textpoint', true).append('text');

join.order();

join.each(function(d) {
var g = d3.select(this);
var sel = transition(g.select('text'));
Expand Down
102 changes: 102 additions & 0 deletions test/jasmine/tests/scatter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,108 @@ describe('end-to-end scatter tests', function() {
.catch(fail)
.then(done);
});

function _assertNodes(ptStyle, txContent) {
var pts = d3.selectAll('.point');
var txs = d3.selectAll('.textpoint');

expect(pts.size()).toEqual(ptStyle.length);
expect(txs.size()).toEqual(txContent.length);

pts.each(function(_, i) {
expect(d3.select(this).style('fill')).toEqual(ptStyle[i], 'pt ' + i);
});

txs.each(function(_, i) {
expect(d3.select(this).select('text').text()).toEqual(txContent[i], 'tx ' + i);
});
}

it('should reorder point and text nodes even when linked to ids (shuffle case)', function(done) {
Plotly.plot(gd, [{
x: [150, 350, 650],
y: [100, 300, 600],
text: ['apple', 'banana', 'clementine'],
ids: ['A', 'B', 'C'],
mode: 'markers+text',
marker: {
color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)']
},
transforms: [{
type: 'sort',
enabled: false,
target: [0, 1, 0]
}]
}])
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', true);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 0, 255)', 'rgb(0, 255, 0)'],
['apple', 'clementine', 'banana']
);

return Plotly.restyle(gd, 'transforms[0].enabled', false);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);
})
.catch(fail)
.then(done);
});

it('should reorder point and text nodes even when linked to ids (add/remove case)', function(done) {
Plotly.plot(gd, [{
x: [150, 350, null, 600],
y: [100, 300, null, 700],
text: ['apple', 'banana', null, 'clementine'],
ids: ['A', 'B', null, 'C'],
mode: 'markers+text',
marker: {
color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', null, 'rgb(0, 0, 255)']
},
transforms: [{
type: 'filter',
enabled: false,
target: [1, 0, 0, 1],
operation: '=',
value: 1
}]
}])
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', true);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 0, 255)'],
['apple', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', false);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);
})
.catch(fail)
.then(done);
});
});

describe('scatter hoverPoints', function() {
Expand Down