Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions docs/bundle.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion docs/index.html

Large diffs are not rendered by default.

26 changes: 26 additions & 0 deletions examples/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
BranchName,
CounterLabel,
Details,
DonutChart,
DonutSlice,
Flash,
Heading,
Label,
Expand Down Expand Up @@ -171,6 +173,30 @@ const Index = props => (
</Details>
</Box>
</Example>
<Example name='DonutChart'>
<Box mb={2}>
<Heading tag='h2' fontSize={3} mb={1}>With <Text mono>data</Text> prop</Heading>
<DonutChart data={{error: 2, pending: 3, success: 5}} />
</Box>
<Box mb={2}>
<Heading tag='h2' fontSize={3} mb={1}>With <Text mono>DonutSlice</Text> children</Heading>
<DonutChart>
<DonutSlice value={1} state='pending' />
<DonutSlice value={1} state='success' />
<DonutSlice value={1} state='error' />
</DonutChart>
</Box>
<Box mb={2}>
<Heading tag='h2' fontSize={3} mb={1}>With custom <Text mono>fill</Text> colors</Heading>
<DonutChart>
<DonutSlice value={1} fill={theme.colors.purple[0]} />
<DonutSlice value={1} fill={theme.colors.purple[1]} />
<DonutSlice value={1} fill={theme.colors.purple[2]} />
<DonutSlice value={1} fill={theme.colors.purple[3]} />
<DonutSlice value={1} fill={theme.colors.purple[4]} />
</DonutChart>
</Box>
</Example>
<Example name='Flash'>
<Box mb={3}>
<Flash> Flash </Flash>
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"dependencies": {
"@github/octicons-react": "1.2.0-alpha.0a0d8862",
"classnames": "^2.2.5",
"d3-shape": "^1.2.0",
"react": "^16.2.0",
"system-classnames": "^1.0.0-3"
},
Expand Down
79 changes: 79 additions & 0 deletions src/DonutChart.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React from 'react'
import PropTypes from 'prop-types'
import {arc as Arc, pie as Pie} from 'd3-shape'
import theme from './theme'

const {colors} = theme

const fillForState = {
'error': colors.red[5],
'pending': colors.yellow[5],
'failure': colors.red[5],
'success': colors.green[5]
}

function mapData(data) {
return Object.keys(data)
.map((key, i) => (
<DonutSlice key={key} state={key} value={data[key]} />
))
}

const DonutChart = props => {
const {
data,
children = mapData(data),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

size = 40,
} = props

const radius = size / 2
const {
innerRadius = radius / 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} = props

const pie = Pie()
.value(child => child.props.value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const arcData = pie(children)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const arc = Arc()
.innerRadius(innerRadius)
.outerRadius(radius)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

} = child.props
return React.cloneElement(child, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, good call!

d: arc(arcData[i]),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fill,
key: i
})
})

return (
<svg width={size} height={size}>
<g transform={`translate(${radius},${radius})`}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{arcs}
</g>
</svg>
)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a "one export per file" rule?

return <path d={d} fill={fill}>{children}</path>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

DonutChart.propTypes = {
children: PropTypes.arrayOf(DonutSlice)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only allow DonutSlice elements as children.

}

DonutSlice.propTypes = {
d: PropTypes.string,
fill: PropTypes.string,
state: PropTypes.oneOf(Object.keys(fillForState)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

value: PropTypes.number
}

export default DonutChart
export {DonutSlice}
32 changes: 32 additions & 0 deletions src/__tests__/DonutChart.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react'
import DonutChart, {DonutSlice} from '../DonutChart'
import theme from '../theme'
import {render} from '../utils/testing'

describe('DonutChart', () => {
it('renders the data prop', () => {
const donut = render(<DonutChart data={{error: 1}} />)

expect(donut.type).toEqual('svg')
expect(donut.props.width).toEqual(40)
expect(donut.props.height).toEqual(40)
expect(donut.children.length).toEqual(1)

const [g] = donut.children
expect(g.type).toEqual('g')
expect(g.children.length).toEqual(1)

const [slice] = g.children
expect(slice.type).toEqual('path')
expect(slice.props.fill).toEqual(theme.colors.red[5])
})

xit('renders DonutSlice children', () => {
const donut = render((
<DonutChart>
<DonutSlice state='success' value={1} />
</DonutChart>
))
expect(donut.type).toEqual('svg')
})
})
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export { default as ButtonOutline } from './ButtonOutline'
export { default as ButtonLink } from './ButtonLink'

export { default as Details } from './Details'
export { default as DonutChart, DonutSlice} from './DonutChart'

export { default as Heading } from './Heading'
export { default as Label } from './Label'
Expand Down