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.

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

Fix ordering option for the barChart. #766

Merged
merged 1 commit into from
Apr 3, 2015

Conversation

mhodorogea
Copy link
Contributor

Fix the bug: ordering not working on barChart.

If you write in the barChart definition:

  1. chart.ordering(function (d) { return d.data.value; });
    It will order the barChart xAxis by value increasing.
  2. chart.ordering(function (d) { return -d.data.value; });
    It will order the barChart xAxis by value decreasing.
  3. chart.ordering(function (d) { return d.data.key; });
    It will order the barChart xAxis by key, alphabetical order.

@gordonwoodhull
Copy link
Contributor

Thanks! Please try the tests with grunt test; apparently you have missed the empty case.

Also, a test or two for this feature would be appreciated.

@mhodorogea
Copy link
Contributor Author

Now the "grunt test" should work.

@mhodorogea mhodorogea force-pushed the master branch 2 times, most recently from 0165f54 to 8b14d1f Compare December 5, 2014 16:28
@mhodorogea
Copy link
Contributor Author

Some tests have been added to check ordering on barChart.

@gordonwoodhull
Copy link
Contributor

Thanks @mhodorogea!

I think this should fix #772

groups[0].values = _chart._computeOrderedGroups(groups[0].values);
}

return groups.reduce(function (all, layer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite right because it only sorts the first stack. If there are multiple stacks, the latter stacks will get tacked on unsorted, and ruin the order. Probably we need to flatten first, then sort.

@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Jan 17, 2015
gordonwoodhull added a commit that referenced this pull request Apr 3, 2015
for #766, avoid concatening unordered data onto ordered data
(ordering is still kind of a weird concept when dealing with stacked bars)

fixes #772 .ordering not working for bar chart
@gordonwoodhull gordonwoodhull merged commit 8b14d1f into dc-js:master Apr 3, 2015
@gordonwoodhull
Copy link
Contributor

Sigh, it turns out this was still not quite correct, because ordinarily the ordering function takes a regular group {key,value} pair, so the default ordering was not correct and was comparing undefined, which can crash crossfilter (see #909).

I am reverting flattenStacks and changing the definition of _ordinalXDomain to pluck the data from the flattened data and order that, and changing the tests to use ordering functions that work like the other ordering functions (no .data).

gordonwoodhull added a commit that referenced this pull request Apr 13, 2015
running _computeOrderedGroups on the d3.stack data meant that the ordering
function had to have a different signature from usual, peering into the
inner `.data`.

this was causing a crossfilter crash in IE (#909).

so revert that and instead have `_ordinalXDomain` pluck the data and order
that. slightly more efficient for the other uses of `flattenStacks` that
don't need ordering, and actually correct.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants