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

Row chart and Pie chart incorrectly called valueAccessor instead of cappedValueAccessor. #1335

Closed

Conversation

alexcampana
Copy link
Contributor

Row chart and Pie chart incorrectly called valueAccessor instead of cappedValueAccessor. This caused issues when using a custom valueAccessor and a cap.

@gordonwoodhull
Copy link
Contributor

Thanks for the contribution @alexcampana! I really appreciate the extra tests too.

This looks correct to me. However, could you describe more in-depth the problem you were seeing? Was it crashing when trying to apply your custom valueAccessor to the others group?

I'm surprised no one got bit by this before, and I'll have to go back to the commits where cappedValueAccessor was introduced, and see if it wasn't applied everywhere, or if later commits brought back the vanilla valueAccessor, or what.

Any additional info you can provide would be helpful.

@alexcampana
Copy link
Contributor Author

Yes, the chart was crashing in my case.

I use reductio alias mapping, so all of my custom valueAccessors reference those aliased functions. Since the othersGroup value doesn't contain any of my aliased functions and only the final calculated value, the valueAccessor blows up when it tries to call the function.

Let me know if you need anything else.

@gordonwoodhull
Copy link
Contributor

Okay, thanks a lot. Like I said, I'll want to look into the root cause of the issue and the history, and then I'll merge this into the 2.0.* branch.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Sep 6, 2017
@gordonwoodhull
Copy link
Contributor

Thanks @alexcampana! Sorry for the delay; I am finally merging this for dc.js 3.0.7.

I looked into why the code was this way, and it looks like

  • in the pie chart I added the "no data" check without thinking about which accessor to use
  • the row chart was updated to cappedValueAccessor in bb20d45 but this one line was missed

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.

None yet

2 participants