d3 / d3-shape

Graphical primitives for visualization, such as lines and areas.
https://d3js.org/d3-shape
ISC License
2.48k stars 308 forks source link

Adds cornerRadius parameter to arc #85

Closed luisalima closed 7 years ago

luisalima commented 7 years ago

Ok so this is the follow up from my wrongful PR, #84 . I think it should be correct now. The problem is still the same:

I'm doing a small library for doing charts with react native and I realized that cornerRadius parameter was being ignored in the most recent d3 version (installed via yarn, v4.3.0). Here is a failing example (ecmascript 2015):

  data = [1,2,3];
  const innerRadius = 30;
  const outerRadius = 50;
  const cornerRadius = 10;
  const arcs = d3.shape.pie()(data);
  const arc = d3.shape.arc();
  return arcs.map((a) => {
    return arc({ ...a, innerRadius, outerRadius, cornerRadius });
  }
  );
}

I did some debugging using console.log with the example above and realized that cornerRadius was always set to 0 and whatever was passed in parameters was ignored.

I did the fix that I'm sending in this PR, and it works well for me locally. Is there anything that I might be missing?

Also, I just ran npm run test so my question was a bit stupod... 😊 ooops, sorry about that 😄

Thanks in advance!

mbostock commented 7 years ago

The corner radius defaults to zero. If you want to specify it as a property of data d.cornerRadius, then you need to create an arc generator with the corresponding accessor:

const arc = d3.arc().cornerRadius(d => d.cornerRadius);

If you want it to be a constant value, then set it as the desired constant value:

const arc = d3.arc().cornerRadius(10);

You should probably do this for innerRadius and outerRadius, too, given your example code.

The cornerRadius is treated different than the other properties in that by default it doesn’t check d.cornerRadius, but that’s because the cornerRadius is considered optional; it does not need to be set explicitly. Even when the corner radius is non-zero, it’s typically the same for all arcs being generated, and so it’s better to specify it as a constant than to propagate it through the data.

luisalima commented 7 years ago

@mbostock oooh got it! Of course. Silly me 🙈 Thanks for the super fast reply!