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

make sure _labelOffsetY doesn't become Infinity #1024

Closed
wants to merge 2 commits into from

Conversation

nordfjord
Copy link

make sure _labelOffsetY doesn't become Infinity and spew a ton of errors to the console.

height becomes Infinity when n == 0, meaning any time the chart is rendered with no data.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Oct 14, 2015
@gordonwoodhull
Copy link
Contributor

Thanks @nordfjord! Much appreciated.

@gordonwoodhull
Copy link
Contributor

Hmm, started to write a test case for this, but I can't figure out: if there are no rows, then how is _labelOffsetY used at all? I.e. maybe it's infinity but then what's it being applied to?

@nordfjord
Copy link
Author

Sorry for the late reply (it's hard to sift through all the emails I receive from github to find the relevant ones).

This bug is not really a bug in the sense that it doesn't break anything, the only thing that happens is it fills my console up with an annoying amount of errors, which I tend to try to avoid.

To be honest I don't actually remember what my test case was. I think I was using a rowchart whose group filtered out 0 values, and when I applied filters so that no values were left in the group my console started spewing tons of errors. I then added this check and like magic my console was clean again.

@Fil
Copy link
Contributor

Fil commented May 30, 2016

My fix was this one:

--- a/js/dc.js
+++ b/js/dc.js
@@ -8487,7 +8487,7 @@ dc.rowChart = function (parent, chartGroup) {
     }

     function updateElements (rows) {
-        var n = _rowData.length;
+        var n = _rowData.length || 1;

         var height;
         if (!_fixedBarHeight) {

(Wish it had been merged. I've just spent 2 hours on this same issue :-/)

@gordonwoodhull
Copy link
Contributor

@Fil, I still don't see how Infinity can be applied to any bars if there aren't any bars. What am I missing? Do you have a test case?

@gordonwoodhull
Copy link
Contributor

Aha! I think I found it. updateElements is getting called at the end of redraw, but the variable is used in updateLabels, which is called twice, both in createElements and in updateElements. So the time after the chart is emptied out, it will update the labels wrong and then fix them.

This also would explain why it spews errors but draws correctly.

So I think the correct fix is to remove the first, redundant call from createElements.

Sorry for being obstinate about not taking fixes without tests or explanations. I feel that it only makes the code harder to read.

@Fil
Copy link
Contributor

Fil commented May 30, 2016

I can confirm that "to remove the first, redundant call from createElements" fixes it too for me.

@Fil
Copy link
Contributor

Fil commented May 30, 2016

(And I totally agree with your being obstinate about getting to the real cause of any bugs rather than just routing around.)

@gordonwoodhull
Copy link
Contributor

Thanks @Fil! I'll put that in the next release.

@nordfjord
Copy link
Author

Shouldn't we close this PR since it's not the real cause of the error?

@nordfjord nordfjord closed this Jul 8, 2016
@gordonwoodhull
Copy link
Contributor

Thanks, I was looking for this issue to include the fix in the last release, but I didn't find it in time.

I'm going to reopen - the description is rarely the root cause!

@gordonwoodhull gordonwoodhull reopened this Jul 8, 2016
@gordonwoodhull
Copy link
Contributor

Ref: #1008

gordonwoodhull added a commit that referenced this pull request Jul 22, 2016
and add a test that invokes the error spew of #1008
wish I could observe the errors from Jasmine!!

fixes #1008
closes #1024
@gordonwoodhull
Copy link
Contributor

Fixed in 2.0 beta 32. Thanks @nordfjord and @Fil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants