Skip to content

Commit

Permalink
Sort null nodes at the end.
Browse files Browse the repository at this point in the history
Previously, null nodes were passed to the comparator and were indistinguishable
from non-null nodes with no bound data. For consistency with other selection
operators that skip null nodes, it seems preferrable to put null nodes at the
end of the selection rather than passing them to the comparator. Fixes d3#881.
  • Loading branch information
mbostock committed Feb 6, 2013
1 parent 183060d commit 2a0e2a1
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 9 deletions.
2 changes: 1 addition & 1 deletion d3.js
Original file line number Diff line number Diff line change
Expand Up @@ -1805,7 +1805,7 @@
function d3_selection_sortComparator(comparator) {
if (!arguments.length) comparator = d3.ascending;
return function(a, b) {
return comparator(a && a.__data__, b && b.__data__);
return !a - !b || comparator(a.__data__, b.__data__);
};
}
d3_selectionPrototype.on = function(type, listener, capture) {
Expand Down
2 changes: 1 addition & 1 deletion d3.min.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/core/selection-sort.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ d3_selectionPrototype.sort = function(comparator) {
function d3_selection_sortComparator(comparator) {
if (!arguments.length) comparator = d3.ascending;
return function(a, b) {
return comparator(a && a.__data__, b && b.__data__);
return (!a - !b) || comparator(a.__data__, b.__data__);
};
}
33 changes: 27 additions & 6 deletions test/core/selection-sort-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,34 @@ suite.addBatch({
assert.deepEqual(span[2].map(data), [1, 10, 12, 30]);
assert.deepEqual(span[3].map(data), [1, 10, 22, 40]);
},
"treats null nodes as null data": function(span) {
var some = d3.selectAll("div:first-child span"), nulls = false;
some[0][0] = null;
some.sort(function(a, b) { if ((a === null) || (b === null)) ++nulls; return a - b; });
assert.isTrue(nulls > 0);
"sorts null nodes at the end of the selection": function() {
var span = d3.selectAll("div").selectAll("span"), nulls = 0;
d3.select(span[0][0]).remove();
span[0][0] = null;
span.sort(function(a, b) { if ((a === null) || (b === null)) ++nulls; return a - b; });
assert.equal(nulls, 0);

assert.isNull(span[0][3]);
assert.domNull(span[0][0].previousSibling);
assert.deepEqual(some[0].slice(1).map(data), [3, 10, 21]);
assert.domEqual(span[0][1], span[0][0].nextSibling);
assert.domEqual(span[0][0], span[0][1].previousSibling);
assert.domEqual(span[0][2], span[0][1].nextSibling);
assert.domEqual(span[0][1], span[0][2].previousSibling);
assert.domNull(span[0][2].nextSibling);
assert.deepEqual(span[0].slice(0, -1).map(data), [3, 10, 21]);

for (var i = 1; i < 4; ++i) {
var d = span[i].parentNode.__data__;
assert.domNull(span[i][0].previousSibling);
assert.domEqual(span[i][1], span[i][0].nextSibling);
assert.domEqual(span[i][0], span[i][1].previousSibling);
assert.domEqual(span[i][2], span[i][1].nextSibling);
assert.domEqual(span[i][1], span[i][2].previousSibling);
assert.domEqual(span[i][3], span[i][2].nextSibling);
assert.domEqual(span[i][2], span[i][3].previousSibling);
assert.domNull(span[i][3].nextSibling);
assert.deepEqual(span[i].map(data), [1, 2 + d, 10, 20 + d].sort(d3.ascending));
}
},
"returns the current selection": function(span) {
span = d3.select("body"); // https://github.com/tmpvar/jsdom/issues/277
Expand Down

0 comments on commit 2a0e2a1

Please sign in to comment.