dc.js icon indicating copy to clipboard operation
dc.js copied to clipboard

re-implement cap mixin for sunburst chart

Open alexnb opened this issue 5 years ago • 4 comments

In #1573, capMixin was removed from sunburst. Could you please also remove the documentation reference to it ( * @mixes CapMixin)

Thanks and best regards, Alex

alexnb avatar Feb 16 '20 16:02 alexnb

Hi @alexnb, thanks for the report.

Did you want this feature to work? As I mentioned in the linked PR, I wasn’t sure if it was useful, and it had a bug which no one reported, so I guessed no one was using it.

If it would be useful, we could restore and fix it instead.

gordonwoodhull avatar Feb 16 '20 16:02 gordonwoodhull

Hi @gordonwoodhull. Just to have "Others" in the inner ring, I think we do not need it. In case this would work for every ring, we would like to use it very much, since sometimes there are very many "subsectors" which deserve to be placed in "Others" to reduce "rubbish" in the chart.

And, we are still with dc.js 3, because we need to support IE10 :(

alexnb avatar Feb 16 '20 17:02 alexnb

I see. Yeah, in principle having "others" for every ring (for every tree branching, really) shouldn't be all that hard to implement, but it would have nothing and it would have much in common with the capMixin.

gordonwoodhull avatar Feb 17 '20 10:02 gordonwoodhull

I've changed my mind. I think it should be easy to use the cap mixin for all sets of children in a sunburst chart.

Here is the relevant code:

https://github.com/dc-js/dc.js/blob/92c897a47acc333c2c11fd1f2ca7ca69c0886429/src/base/cap-mixin.js#L39-L64

It overrides baseMixin.data() to return capped data.

The cap mixin doesn't have any state that would stop it from being used on each set of children. Its state consists of information about how to sort and group, but it doesn't store any data itself.

I think the above code could be refactored as so:

  1. take the code after group.all() and put it in a method that takes an array of slices. Call it say, capMixin.transformChildren
  2. stop overriding .data() at this level. Move the code that checks whether to cap and fetches the data to another method, say capMixin.capGroupData, that takes a group and calls transformChildren if capping is active.
  3. this.data(this.capGroupData); in the row chart and pie chart.

Now the sunburst chart could use transformChildren on each bundle of children, while keeping the row and pie charts working as before.

gordonwoodhull avatar Feb 26 '20 00:02 gordonwoodhull