Skip to content
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

Jump to bottom

Selection re-ordering for scatter traces with ids #1709

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 2 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
4 changes: 4 additions & 0 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
}
});

join.order();

if(hasTransition) {
join.exit().transition()
.style('opacity', 0)
Expand Down Expand Up @@ -483,6 +485,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
});
});

join.order();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆘 IMPORTANT 🆘

This commit breaks scatter/plot.js for traces with non-numeric (x,y) positions.

That is due to Drawing.translatePoint which is called on the enter and merge selections. When points with non-numeric (x,y) are detected, Drawing.translatePoint removes them. Unfortunately, this makes the join selections out-of-sync with the DOM and leads to 'TypeError: Cannot read property 'textContent' of null exception thrown by d3.

So, I have two questions for the d3-heads looking at this PR:

  • Is there a way to re-select the join and call order() on that selection?
  • Or should we make our updates more d3-esque by filtering data items with non-numeric position before binding them to the selections?

Copy link
Contributor

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

Choose a reason for hiding this comment

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

How about join.filter(onlyNumeric).order()?

I'm not sure I understand how makePoints works.

Shouldn't these lines:

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

be:

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

Otherwise DOM and data won't be in sync. Here's a codepen showing a similar issue.


join.exit().remove();
}

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