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

implement sunburst.relativeRingSizes in dc 3 to provide more flexible… #1655

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

the3ver
Copy link
Contributor

@the3ver the3ver commented Feb 18, 2020

… ring sizing; fixes #1511

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Feb 18, 2020

Thanks @the3ver! Very nice and clean way to factor out the changing behavior, and I'm glad it defaults to the old behavior. Thank you so much for writing tests!

Since it is a whole ring sizes interface that gets replaced, what do you think about going one step further and opening it up so that people can replace all of it if they want?

I am imagining someone showing up next year saying "ah but there is a third strategy!" The "relative ring sizes" strategy you came up with looks really general, but there is always someone who comes along with another idea.

(We always try to keep everything open and customizable in dc.js. It is a leaky abstraction by design.)

So how about..?

  • make a new public member chart._defaultRingSizes with the defaults currently on_ringSizes, then initialize _ringSizes = this._defaultRingSizes
  • make a new public member _relativeRingSizes with the stuff that is currently initialized in relativeRingSizes
  • new getter/setter ringSizes that changes _ringSizes instead of the one that just changes the relativeRingSizesFunction

If desired, also have a constructor for relativeRingSizes. With ES6 users could just do

chart.ringSizes({...chart._relativeRingSizes, relativeRingSizesFunction: r => { ... }})

but with ES5 is it would be the more annoying

chart.ringSizes(Object.assign({},
                              chart._relativeRingSizes,
                              {relativeRingSizesFunction: function(r) { ... }}));

The change looks weird in DC3 but it is forward-compatible: in DCv3 there aren't members exposed publicly like this, but in DCv4 all the "private" members are exposed as this._whatever.

- added web/examples/sunburst-equal-radii.html
@the3ver
Copy link
Contributor Author

the3ver commented Feb 19, 2020

@gordonwoodhull nice suggestions, thanks, I think this makes the interface clearer and more consistent. Also the dc.sunburstChart.RelativeRingSizes constructor is very useful.

I think you meant _defaultRingSizes (and _relativeRingSizes) to be an object, not a function, now that i look at the code again. Please let me know, if thats the case.

@gordonwoodhull
Copy link
Contributor

I like this! It's as powerful as what I was suggesting, but a simpler interface.

As you show, there is no reason to expose the underlying objects, just provide constructors for the different ring sizing strategies. Then if people want to change the behavior they overwrite whatever they want on the strategy object.

I will make a few small changes (e.g. RelativeRingSizes is not a class, it's a function that returns an object, so it can be an ordinary method) and then merge for 3.2.0.

Thanks for your contribution!

gordonwoodhull added a commit that referenced this pull request Mar 4, 2020
for #1655
the various constructors are ordinary methods with non-prefixed names
document RingSizes and various other documentation cleanup
@gordonwoodhull gordonwoodhull merged commit d45f777 into dc-js:support/3.x Mar 4, 2020
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 5, 2020

Released in 3.2.0 and 4.0.2. Thanks @the3ver!

@the3ver the3ver deleted the the3ver-relativeRingSizes branch March 11, 2020 14:33
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.

2 participants