Fix Series Chart examples

dc-js / dc.js

Multi-Dimensional charting built to work natively with crossfilter rendered with d3.js

Apache License 2.0

7.41k stars 1.81k forks source link

Fix Series Chart examples #1781

Closed kum-deepak closed 3 years ago

kum-deepak commented 3 years ago

Before moving deeper into dimension related work, I wanted to ensure that related examples work. While going through these I realized that Series chart can be used in far more ways than what I had considered.

I had assumed, incorrectly, that only Stack based charts can be used as children in Series charts. The final solution that I came up with involves following:

groupName is now part of dataProvider. Very few charts use it and ealier it was tangled with group and stack which is no longer the case.
Updated CFMultiAdaptor to consider group as the first layer if it is set. This in my opinion gives best of the world. If someone wants to initialize just one layer, they can alternatily configure it like most of the other charts by setting the group. However if they want more than one layer they need to set layers. Unlike earlier, group is not mandatory.

gordonwoodhull commented 3 years ago

Hi @kum-deepak! If you want me to review this, could you please articulate the problem as well as the solution?

It's difficult to review code without knowing its purpose.

Yes, any coordinate grid chart can be used in a Series chart. I guess that affected the design of this PR, but it doesn't tell me what the PR is for.

It looks like this PR has something to do with the inconsistency of having to using .group() for the first, and .stack() for the rest (ref #797). But I don't understand the connection to the series chart examples, or what needs to be fixed about them.

kum-deepak commented 3 years ago

You did pick up the correct one without me being clear. I will put more details:

In the current dc version, the first layer is set using group and subsequent ones using stack.
In this branch, before this PR, to provide data for Stack based charts, layers was need to be set.
In Series chart, I had assumed that only stack based charts would get used and I was setting layers for the children.
I initially thought of wrtting two cases within SeriesChart, the solution was not going to be clean.
So, switched to allowing optionally setting group for Stack based charts.
This allowed Series chart to always set group and groupName, which now works for all charts. (https://github.com/dc-js/dc.js/pull/1781/files#diff-e17c339ca07e590ef938f6c67d08fd493d11f5d428aa7e289ced43bb6e81a2eeR101)
With this PR now someone can set layers, group, or, both.

gordonwoodhull commented 3 years ago

Thanks, I think I get it.

I would have thought that the scatter series example would cover this case. Was it not working, or working by accident, before this fix?

kum-deepak commented 3 years ago

I realize that the next set of work around dimensions is going to break test cases and charts. So, before getting deeper I wanted to check if all examples work. That is when I found issues with the Cumulative Line Chart and Scatter Series were not working. During the fix I further realized that there were few minor issues in the current develop branch which became part of https://github.com/dc-js/dc.js/pull/1780.

It was the Scatter Series example was miserably failing which gave me idea that something fundamental has gone wrong, I think I am getting some hang of this library :smile: