Add DonutChart component #55

shawnbot · 2018-06-05T17:17:06Z

The DonutChart renders a little chart that looks like this, from the Merge Box:

The React component takes either a data prop, which is an object literal in the form {[state]: value}, e.g.

<DonutChart data={{success: 3, error: 2}} />
<DonutChart data={{pending: 1, success: 4}} />

Or, if you need more control, you can pass the exported DonutSlice component as children with value and either state or fill props:

<DonutChart>
  <DonutSlice value={4} state='pending' key='pending' />
  <DonutSlice value={1} state='success' key='success' />
  <DonutSlice value={3} fill={someCustomColor} key='other' />
</DonutChart>

Under the hood, this uses the pie generator from d3-shape, which has a really nice API. ✨

TODO

Get sizing and inner radius to match the existing component
Check that we're using the correct colors
Get DonutSlice test passing
Review

shawnbot · 2018-06-05T17:19:19Z

As of 8288ef1 it looks like this on Pages:

shawnbot · 2018-06-05T17:22:56Z

src/DonutChart.js

+const DonutChart = props => {
+  const {
+    data,
+    children = mapData(data),


If the children prop isn't passed, generate slices from the data prop.

shawnbot · 2018-06-05T17:23:56Z

src/DonutChart.js

+
+  const radius = size / 2
+  const {
+    innerRadius = radius / 2


Dunno if we want the inner radius of the donut (the size of the "hole") to be fixed or relative. The way it's written write now, it can be passed via the innerRadius prop, which is probably not what we want.

shawnbot · 2018-06-05T17:24:25Z

src/DonutChart.js

+  } = props
+
+  const pie = Pie()
+    .value(child => child.props.value)


Translation: create a pie slice generator with a value accessor that gets props.value from each child.

shawnbot · 2018-06-05T17:24:44Z

src/DonutChart.js

+  const pie = Pie()
+    .value(child => child.props.value)
+
+  const arcData = pie(children)


Translation: generate the pie slice data from the children array.

shawnbot · 2018-06-05T17:25:29Z

src/DonutChart.js

+  const arcData = pie(children)
+  const arc = Arc()
+    .innerRadius(innerRadius)
+    .outerRadius(radius)


We use a custom arc generator here because we need to set the inner and outer radii to fixed values.

shawnbot · 2018-06-05T17:26:51Z

src/DonutChart.js

+  const arcs = React.Children.map(children, (child, i) => {
+    const {
+      state,
+      fill = fillForState[state]


The fill prop can be passed as a string; if it's not passed, we get the color from the corresponding key of fillForState. We should probably have a default here, e.g. theme.colors.gray[5].

shawnbot · 2018-06-05T17:27:56Z

src/DonutChart.js

+      fill = fillForState[state]
+    } = child.props
+    return React.cloneElement(child, {
+      d: arc(arcData[i]),


The d attribute is the SVG path's actual shape, which arc() generates for us from the corresponding entry of the pie slice data.

shawnbot · 2018-06-05T17:28:47Z

src/DonutChart.js

+
+  return (
+    <svg width={size} height={size}>
+      <g transform={`translate(${radius},${radius})`}>


Pie paths are always centered on (0, 0), so we need to translate to the center.

shawnbot · 2018-06-05T17:29:27Z

src/DonutChart.js

+}
+
+const DonutSlice = ({children, d, fill}) => {
+  return <path d={d} fill={fill}>{children}</path>


Note that we pass through children here so that we can add an SVG <title>.

shawnbot · 2018-06-05T17:29:52Z

src/DonutChart.js

+}
+
+DonutChart.propTypes = {
+  children: PropTypes.arrayOf(DonutSlice)


Only allow DonutSlice elements as children.

shawnbot · 2018-06-05T17:33:29Z

src/DonutChart.js

+}
+
+DonutChart.propTypes = {
+  children: PropTypes.arrayOf(DonutSlice),


Only allow DonutSlice elements as children.

shawnbot · 2018-06-05T17:33:47Z

src/DonutChart.js

+
+DonutChart.propTypes = {
+  children: PropTypes.arrayOf(DonutSlice),
+  data: PropTypes.objectOf(PropTypes.number),


Data should have values that are numbers.

shawnbot · 2018-06-05T17:34:12Z

src/DonutChart.js

+DonutSlice.propTypes = {
+  d: PropTypes.string,
+  fill: PropTypes.string,
+  state: PropTypes.oneOf(Object.keys(fillForState)),


Validate allowed values for the state prop based on the keys of our fillForState object.

shawnbot · 2018-06-05T19:04:18Z

Okay, this is ready for a final review. Note that in 44f5a9d I had to employ a variant of this hack to make DonutChart's propTypes allow one or more DonutSlice children. (TIL: when you only have one child, children is not an array!)

emplums

Just left a few comments, but overall this is looking great! 🎉

emplums · 2018-06-05T18:19:14Z

src/DonutChart.js

+const DonutChart = props => {
+  const {
+    data,
+    children = mapData(data),


emplums · 2018-06-05T18:30:44Z

src/DonutChart.js

+  )
+}
+
+const DonutSlice = ({children, d, fill}) => {


Although DonutChart and DonutSlice are fairly tied together, I still think it makes sense to put DonutSlice in a separate file and import it. Makes it easier to find in the file system if we need to make changes, simplifies this file a bit, etc. How do we feel about making that a general best practice (keep each component in a separate file)? cc @shawnbot @broccolini @jonrohan

Maybe a "one export per file" rule?

emplums · 2018-06-05T18:37:28Z

src/DonutChart.js

+      state,
+      fill = fillForState[state] || DEFAULT_FILL
+    } = child.props
+    return React.cloneElement(child, {


This is cloning a DonutSlice correct? Wonder if it would be more declarative here to instead do:

return <DonutSlice d={arc(arcData[i]} fill={fill} key={i}/>

since we don't need access to the refs?

Oh yeah, good call!

emplums · 2018-06-05T18:39:41Z

src/DonutChart.js

+}
+
+DonutChart.propTypes = {
+  children: PropTypes.arrayOf(DonutSlice),


General component API thoughts:

I like using PropTypes to check types here and including it at the bottom of every component file, should we start doing this for every component?

Should we start setting isRequired in our prop types declarations? I think in this component, the component would break if data is not passed in, correct?

shawnbot · 2018-06-05T20:18:28Z

Thanks, @emplums! I moved DonutSlice into its own file, which nicely simplified DonutChart and compartmentalized all of the color logic to DonutSlice so that DonutChart doesn't have to know anything about it. It'd be nice to be able to move all of the geometry stuff into DonutSlice eventually, but I think this is good enough for now. ✌️

shawnbot added 3 commits June 5, 2018 10:08

DonutChart + DonutSlice

7e87871

basic DonutChart test(s)

2255eba

update docs w/DonutChart

8288ef1

shawnbot requested review from emplums and jonrohan June 5, 2018 17:17

shawnbot deployed to github-pages June 5, 2018 17:17 View deployment

shawnbot commented Jun 5, 2018

View reviewed changes

shawnbot added 2 commits June 5, 2018 10:30

add default fill for invalid states

c87286b

add more DonutChart propTypes

3b6415c

shawnbot deployed to github-pages June 5, 2018 17:33 View deployment

shawnbot commented Jun 5, 2018

View reviewed changes

DonutChart default size = 30; fixed inner radius

3a11bda

shawnbot deployed to github-pages June 5, 2018 18:30 View deployment

shawnbot added 3 commits June 5, 2018 11:56

more DonutChart examples

0d3b90a

better DonutChart colors

ade2745

fix DonutChart children propTypes, work w/a single DonutSlice

44f5a9d

shawnbot deployed to github-pages June 5, 2018 18:57 View deployment

more DonutChart tests

ec2e10e

shawnbot deployed to github-pages June 5, 2018 19:01 View deployment

shawnbot changed the title ~~[WIP] Add DonutChart component~~ Add DonutChart component Jun 5, 2018

update docs

d164ff6

shawnbot deployed to github-pages June 5, 2018 19:06 View deployment

emplums approved these changes Jun 5, 2018

View reviewed changes

move DonutSlice into its own file /cc @emplums

f6bf27a

shawnbot deployed to github-pages June 5, 2018 20:15 View deployment

shawnbot changed the base branch from master to release-0.0.3-beta June 5, 2018 20:18

shawnbot merged commit bd03f04 into release-0.0.3-beta Jun 5, 2018

shawnbot deleted the donut-chart branch June 5, 2018 20:18

shawnbot mentioned this pull request Jun 5, 2018

v0.0.3-beta release tracking #54

Merged

10 tasks

Add DonutChart component #55

Add DonutChart component #55

Uh oh!

Conversation

shawnbot commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

shawnbot commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnbot commented Jun 5, 2018

Uh oh!

emplums left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnbot commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shawnbot commented Jun 5, 2018 •

edited

Loading