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

Don't try to interpolate random data in label paths #1151

Closed
wants to merge 1 commit into from

Conversation

stillesjo
Copy link
Contributor

At the moment when there is no data available for the pie chart dc.js will provide the value 1 as default, along with the emptyTitle (which can be set). I've experienced a problem when I have objects assigned as "value" in dc.js, with a value accessor that digs more deeply inside that object to find a value. When I then change filter so that it returns an empty set of data it will try to interpolate with my object (e.g. { value: 1}) and the empty value (e.g. 1). Interpolating between an object and a number will return NaN which will cause the charts to become unusable until refresh of webpage.

This PR gives you the possibility to set the value that should be used when no data is available after filtering. Then you can set it to an object which can be correctly interpolated to and from.

Another problem while developing this was that the key and value accessors for charts inheriting from the capMixin (cappedKeyAccessor/cappedValueAccessor) bothers with the others field in the object. Before this PR we automatically set it to an array containing the emptyTitle-variable. In order to be able to have the value accessed with the value accessor I also added the possibility to set "emptyOthers" to be used in the others field.

Added TC for emptyValue with both a number and an object. I was unable to create a good test case for the others field since it basically never happens since we have no data, so all cap options that I tried wouldn't trigger it to show the others tags; it always resulted in emptyTitle.

Another solution to this could be to combine title, value and others in an object and make it possible to set this object via only one method (instead of three as it is in this PR)

@stillesjo
Copy link
Contributor Author

Similar to #1128. Care to review @XaserAcheron ?

@gordonwoodhull
Copy link
Contributor

I feel there should be a simpler solution to this, as in #1128 where we only tween the data we need.

@stillesjo
Copy link
Contributor Author

The problem is that the values we tween between isn't consistent with eachother. In my case the value we go from is an object and the "empty value" is a Number. This will result in NaN which will break the charts.

Do you have any suggestion for how it could be done otherwise to avoid NaNs?

@stillesjo stillesjo closed this Jun 1, 2016
@stillesjo stillesjo reopened this Jun 1, 2016
@gordonwoodhull
Copy link
Contributor

The heart of #1128 is that we were tweening data which didn't need to be tweened. We don't actually need the data to be tweened, just the angles.

I haven't looked into this in-depth but my feeling is that there is probably the same root problem here: tweening an object we don't eventually use.

@gordonwoodhull
Copy link
Contributor

My other thought is that perhaps we don't need to call pie in this case, since we're basically passing it constants. We could just produce the post-pie-like data instead.

@XaserAcheron
Copy link
Contributor

Is the NaN-tween issue still happening in beta 28 or later? #1128 ought to have this fixed unless I missed an edge case or it's an eerily-similar-but-different issue. As @gordonwoodhull mentioned, the fix made it so the pie tweening doesn't care about or look at the data object, so the nefarious attempted-tweening of 1 and {} should no longer occur.

The PR likely has value for the other reasons mentioned in the first post, though. I think I've got my own workarounds for similar situations in my own project, but I'd have to spelunk through the code to be sure since my memory is notoriously zany.

@stillesjo
Copy link
Contributor Author

Hi again,
Thanks for you comments. I've been trying to reproduce the fault and noticed that the fault doesn't occur on tweenPie but in fact happens if you have external labels enabled, in the updateLabelPaths function in pie-chart.js.

I'll try implementing a similar solution there as in #1128 (i.e. only interpollating on only the angles) and see if it works.

You can check out the fault in this codepen by filtering in the line chart to give no data and then removing the filter. The fault is displayed in the log.

@stillesjo
Copy link
Contributor Author

@gordonwoodhull @XaserAcheron
I've updated the PR with a similar solution as #1128 for updateLabelPaths. I've also tested this towards our application and it solves the problem. Please let me know what you think 🍰

@XaserAcheron
Copy link
Contributor

Oh heh, that explains it -- I'm not using external labels in the project I identified the issue in, so I missed it. Thanks for the patch-up!

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Jul 27, 2016
@stillesjo
Copy link
Contributor Author

@gordonwoodhull
Did I miss any comment/stuff on this PR or could this be merged into develop?

@gordonwoodhull
Copy link
Contributor

Yes, it looks like this can be merged into the 2.0 betas. I've just overlooked it. Thanks!

@gordonwoodhull gordonwoodhull changed the title Pie-Chart: Set value and others to be used when no data is available Don't try to interpolate random data in label paths Dec 1, 2016
@gordonwoodhull
Copy link
Contributor

Merged for 2.0.0-beta.33. Thanks @stillesjo!

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.

3 participants