Skip to content

Fix Series Chart examples #1781

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

Merged
merged 2 commits into from
Nov 7, 2020
Merged

Fix Series Chart examples #1781

merged 2 commits into from
Nov 7, 2020

Conversation

kum-deepak
Copy link
Collaborator

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.

@kum-deepak kum-deepak added this to the dc-v5 milestone Nov 3, 2020
@gordonwoodhull
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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?

Copy link
Contributor

@gordonwoodhull gordonwoodhull left a comment

Choose a reason for hiding this comment

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

Sound good! Thanks for all your efforts!

@kum-deepak
Copy link
Collaborator Author

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 #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 😄

@kum-deepak kum-deepak merged commit a90ea73 into dc-js:dc-v5 Nov 7, 2020
@kum-deepak kum-deepak deleted the series-fix branch November 7, 2020 15:28
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.

2 participants