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

Include 0 as max in elastic axis if all values are negative #1156

Closed
wants to merge 4 commits into from

Conversation

sebgrohn
Copy link
Contributor

@sebgrohn sebgrohn commented Jun 7, 2016

For Row chart and charts using Stack mixin, use the corresponding method for determining the max value for the y axis as used for min value:

yAxisMin, current behaviour: If all values are positive, use 0 as y axis min.
yAxisMax, new behaviour: If all values are negative, use 0 as y axis max.

Should fix issue #879.

Two Jasmine tests are currently failing, both related to Composite chart and its alignYAxes. As far as I can tell, it seems to be related to the calculations in calculateYAxisRanges that does something magic with the y axis ranges if there are two y axes and alignYAxes are set. I am at loss here what to do, all suggestions are welcome!

jasmine-failing-tests.txt

@sebgrohn sebgrohn closed this Jun 7, 2016
@sebgrohn sebgrohn changed the title Fix only negative values Include 0 as max in y axis scale if all values are negative Jun 7, 2016
@sebgrohn sebgrohn changed the title Include 0 as max in y axis scale if all values are negative Include 0 as max in y axis if all values are negative Jun 7, 2016
@sebgrohn sebgrohn reopened this Jun 7, 2016
@gordonwoodhull
Copy link
Contributor

@sebgrohn, this looks like an excellent approach. I like the symmetry of it.

Would you be willing to also write/improve a few tests for this?

In particular, the row chart already has tests for all-negative values, but it looks like it doesn't test elasticX.

The bar chart apparently does test elasticY with negative values, but it didn't detect any problems. Ditto for the line chart.

Don't worry about alignYAxes since that feature is still somewhat in flux and #1033 may help there.

@sebgrohn
Copy link
Contributor Author

Great! I will try to produce some tests for negative values with elasticY enabled. I can't give any time frame right now, though.

sebgrohn and others added 3 commits June 20, 2016 16:29
…values negative

Use the same algorithm as for the y axis minimum in stackMixin, so that
zero is always included in the y axis domain.
…are negative

Do the same check in rowChart for the x axis maximum as for the
minimum, so that zero is always included in the x axis domain.
@sebgrohn
Copy link
Contributor Author

sebgrohn commented Jun 20, 2016

Added tests for Row, Bar, and Line chart that checks for the correct x/y axis ticks when elasticX / elasticY is enabled.

Notes:

  • I added the missing Line chart tests for enabled elasticX, all three cases (all positive, mixed, all negative). I'm not sure, however, if they should be included in the template function itShouldBehaveLikeARowChartWithGroup or not...
  • The Bar and Line chart tests only tested with mixed-sign values, so I renamed them and added new ones for all-negative values.

@gordonwoodhull and others: comments on this?

@gordonwoodhull gordonwoodhull changed the title Include 0 as max in y axis if all values are negative Include 0 as max in elastic axis if all values are negative Jul 14, 2016
@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Jul 27, 2016
it('should generate x axis domain dynamically', function () {
var nthText = function (n) { return d3.select(chart.selectAll('g.axis .tick text')[0][n]); };

for (let i = 0; i < xAxisTicks.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Crazy... all your code works well, but for some reason the let here caused Jasmine to skip the row chart tests! Completely silent failure!

dc.js is still ES5 and will be for a while yet. @sebgrohn, did you have to do anything special to get the tests to run?

Of course, I noticed 58 commits later that my test count was way down. Easy enough to fix, but whoah that's a little scary if one ES6 keyword causes a file to get dropped entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, that's interesting (and scary)! Sloppy to let a let (sic.) slip in there; I am truly ES6 damaged.

I don't remember if I did something out of the ordinary, and I have the code and dev env set up on another computer I can't access right now... :o

@gordonwoodhull
Copy link
Contributor

Thanks @sebgrohn, rebased and merged in 2.0 beta 32!

@sebgrohn
Copy link
Contributor Author

Great!

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