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

Implemented d3.svg.arc.cornerRadius #1132

Closed
wants to merge 3 commits into from
Closed

Implemented d3.svg.arc.cornerRadius #1132

wants to merge 3 commits into from

Conversation

bm-w
Copy link

@bm-w bm-w commented Mar 10, 2013

See #1131. This implementation uses approach C for the case of large corner radius relative to arc length—i.e. using Bézier curve approximations to both align the corner start points with those of neighbouring slices and also touch the inner and outer radius, at the expense of the corners no longer being circular.

This features adds about 1.3kB to the minified library (comparing the results of make).

NB. I went ahead and implemented this approach, though I'm still open to switching to the other approaches if so desired.

@lgrkvst
Copy link

lgrkvst commented Jul 28, 2013

Hi,

I've tested your cornerRadius implementation and it works really good - sleek corners! Thank you for doing this.

The oddest little caveat though - I can't seem to do this:
var d3hull = d3.geom.hull();

This throws the following:
Uncaught TypeError: Cannot read property 'length' of undefined d3.min.bm-w.cornerradius.js:4
zi.geom.hull d3.min.bm-w.cornerradius.js:4
(anonymous function) force_with_labels.js:18

I'm not entiry sure, but it points towards d3.min.bm-w.cornerradius.js as it all works when I change to the "normal" d3.v3.min.js.

It should be easy to reproduce, otherwise, I'm doing something wrong on my end.

Keep up the good work!

Best regards,
Christian

@ruphin
Copy link

ruphin commented Apr 11, 2014

Hi,

This pull request adds a useful feature to D3 Arcs. We are currently using the latest version of D3 with these changes added on top, and haven't encountered any issues. Perhaps someone can take a look at this pull request, it would be nice to have it merged so we can use the main release of D3 again.

With kind regards,
Goffert

@lgrkvst
Copy link

lgrkvst commented Apr 11, 2014

Seconded! :>

@mbostock mbostock modified the milestones: 3.5, Icebox Apr 11, 2014
@chrisvfritz
Copy link

This has been very useful in my projects as well and I also have not encountered any issues.

@terite
Copy link

terite commented May 13, 2014

👍

@mbostock
Copy link
Member

Can I ask you to sign the CLA, per D3’s CONTRIBUTING guidelines?

@bm-w
Copy link
Author

bm-w commented May 31, 2014

@mbostock Signed. Note that this PR is fairly old. I currently haven't got the time to update, but I'm sure the signature leaves anyone else who is interested free to do so (@o-o-, @ruphin, @chrisvfritz, @terite?).

@mbostock mbostock mentioned this pull request Nov 14, 2014
@mbostock
Copy link
Member

Finally looking at this now for 3.5. I love the diagram you made illustrating the different approaches. I think I’m inclined to approach B — truncating the radius if the arc is too thin — based on it being similar to how SVG treats rx and ry for rounded rects (per @deanmalmgren’s comment).

Also for backwards-compatibility (and based on expected usage), I think it makes sense to have the cornerRadius default to zero rather than default to an accessor like the other properties.

I plan on working on integrating this in the next few days.

@chrisvfritz
Copy link

🎉 !

@mbostock
Copy link
Member

Fixed in #2118 for 3.5.

@mbostock mbostock closed this Nov 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants